refactor(deploymentmonitor): use SpinnakerRetrofitErrorHandler with DeploymentMonitorService #4628
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbyron-sf
force-pushed
the
deployment-monitor-service-spinnaker-exception
branch
4 times, most recently
from
91fe1be to
c60c839
Compare
to make it possible to add tests that assert on the contents of log messages. orca-test has a logback-test.xml file that disables logging, so add one for orca-clouddriver that takes precedence so log messages actually get generated. Now that logging is enabled, clock.millis() is called an additional time during a test in ServerGroupCacheForceRefreshTaskSpec.
dbyron-sf
force-pushed
the
deployment-monitor-service-spinnaker-exception
branch
from
c60c839 to
21f7b10
Compare
to see what changes when moving to SpinnakerRetrofitErrorHandler. Specifically, these tests verify the contents of a log message that's tightly coupled to RetrofitError.
…e for different types of RetrofitError (spinnaker#4608)" This reverts commit adc81ac. With the addition of EvaluateDeploymentHealthTaskTest, we have equivalent coverage that: - is in java instead of groovy - doesn't increase the visibility of getRetrofitLogMessage - more easily shows behavior changes when adopting SpinnakerRetrofitErrorHandler - is less likely to change when moving from retrofit to retrofit2
…eploymentMonitorService
Note, there's a behavior change here when http response bodies aren't json objects.
Previously, the log message would, barring an exception processing the response, include
the http response body in the log message. With SpinnakerHttpException, response bodies
that aren't json objects aren't available, so the body appears to be empty.
For example, before:
2024-01-04 12:31:59.640 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:49179/deployment/evaluateHealth, status: 400 (Bad Request)
response body: non-json response}
retrofit.RetrofitError: 400 Bad Request
at retrofit.RetrofitError.httpError(RetrofitError.java:40)
after:
2024-01-04 14:26:13.270 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:58192/deployment/evaluateHealth, status: 400 (Bad Request)
response body: }
com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 400, URL: http://localhost:58192/deployment/evaluateHealth, Message: Bad Request
at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55)
There's also a behavior change for ConversionException.
before:
2024-01-04 16:03:19.363 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:60051/deployment/evaluateHealth, status: 200 (OK)
headers:
response body: }
retrofit.RetrofitError: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentStep` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('bogus')
at [Source: (retrofit.ExceptionCatchingTypedInput$ExceptionCatchingInputStream); line: 1, column: 13] (through reference chain: com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse["nextStep"])
at retrofit.RetrofitError.conversionError(RetrofitError.java:33)
after:
2024-01-04 16:14:03.540 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:59402/deployment/evaluateHealth, <NO RESPONSE>}
com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentStep` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('bogus')
at [Source: (retrofit.ExceptionCatchingTypedInput$ExceptionCatchingInputStream); line: 1, column: 13] (through reference chain: com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse["nextStep"])
at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:64)
at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242)
dbyron-sf
force-pushed
the
deployment-monitor-service-spinnaker-exception
branch
from
21f7b10 to
e9b0afc
Compare
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note, there's a behavior change here when http response bodies aren't json objects.
Previously, the log message would, barring an exception processing the response, include
the http response body in the log message. With SpinnakerHttpException, response bodies
that aren't json objects aren't available, so the body appears to be empty.
For example, before:
after:
There's also a behavior change for ConversionException.
before:
after: