-
Notifications
You must be signed in to change notification settings - Fork 805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service #4623
refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service #4623
Conversation
cfc1b7b
to
328d7b8
Compare
7fc719e
to
483b3e0
Compare
483b3e0
to
4d0485f
Compare
ee64ec5
to
d81480a
Compare
3459464
to
fcc2e80
Compare
fcc2e80
to
29643d8
Compare
4c6aa53
to
886e2b1
Compare
13169ef
to
ae24d3f
Compare
5ac2f60
to
59945dd
Compare
orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy
Show resolved
Hide resolved
.../src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/job/WaitOnJobCompletion.groovy
Outdated
Show resolved
Hide resolved
orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/job/JobUtils.java
Outdated
Show resolved
Hide resolved
@@ -466,10 +466,9 @@ public boolean hasDisableLock(Moniker clusterMoniker, String account, Location l | |||
Application application; | |||
try { | |||
application = front50Service.get(clusterMoniker.getApp()); | |||
} catch (RetrofitError e) { | |||
} catch (SpinnakerHttpException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a test in TrafficGuardSpec that doesn't strictly need to change because of what's happening in this PR. It does look like a broken test though...and there's a unnecessary null check of application a few lines below here. Because Front50Service.get returns a "real" type (i.e. not a Response), it never returns null. So it doesn't make sense to have a test that mocks a null return value. Can you clean this up please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a similar thing happens in UpsertApplicationTaskSpec ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating TrafficGuardSpec. The code below still has a null check that doesn't need to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating TrafficGuardSpec. The code below still has a null check that doesn't need to be there.
Are you talking about this line : https://github.com/Pranav-b-7/orca/blob/0c986730d3918caf7b9d8509dea45cb426d9d81a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java#L477 ?
If yes, this check is still relevant, because in the above catch block when the response code is 404 or 403, application variable is assigned to null.
@@ -70,10 +71,12 @@ ExecutionImportResponse createExecution(@RequestBody PipelineExecution execution | |||
try { | |||
application = front50Service.get(execution.getApplication()); | |||
log.info("Importing application with name: {}", application.name); | |||
} catch (RetrofitError e) { | |||
if (e.getKind() == RetrofitError.Kind.HTTP && e.getResponse().getStatus() != 404) { | |||
} catch (SpinnakerHttpException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you have here matches the original logic. I have to wonder if it's really correct though. If the idea is to be silent about 404s, but warn on other things, wouldn't we want to have one catch (SpinnakerServerException)
block that logs a warning for anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this PR is to move to Spinnaker*Exception, so let's leave the logic as it is for now.
59945dd
to
d02575e
Compare
8efeabf
to
7e923cc
Compare
This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for Front50Service. There are no behaviour changes detected with these code changes and hence no additional tests added to demonstrate the functionalities. Few existing tests are modified to align it with the new changes.
81dcef8
to
ebc0f98
Compare
There are 3 APIs invoked inside the try block: 1. Keel Service API - keelService.deleteDeliveryConfig(): With the addition of the commit - c417922, the keelService uses SpinnakerRetrofitErrorHandler. 2. Front50 Service APIs - front50Service.get() and front50Service.delete(): With the addition of the commit - a44b5bb, the front50 service uses SpinnakerRetrofitErrorHandler. and hence with the above mentioned commits, the catch block with RetrofitError becomes irrelevant.
There are total of 5 API calls happening inside the try block: 1. Igor Service API - scmService.compareCommits(): With the addition of the commit - d430b75, the igorService uses SpinnakerRetrofitErrorHandler. 2. Clouddriver APIs (OortService) - cloudDriverService.getByAmiId() - This is invoked twice in the block, and oortService.getServerGroupFromCluster(): With the addition of the commit - 84a7106, the oortService uses SpinnakerRetrofitErrorHandler. 3. Front50 API - front50Service.get(): With the addition of the commit - a44b5bb, the front50Service uses SpinnakerRetrofitErrorHandler. and hence with the above mentioned commits, the catch block with RetrofitError becomes irrelevant.
…Spec The API call 'front50Service.get(application)' mocked/stubbed to return null value is not realistic, because if the application is not available , then this API would throw a 404 HTTP error and thats already handled in the same class: https://github.com/spinnaker/orca/blob/7e923cc05af0f66b952245722aa69c667f685c5c/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy#L464. and hence this test is cleaned up.
…cationTaskSpec The API call 'front50Service.get(application)' mocked/stubbed to return null value is not realistic, because if the application is not present, then this API would throw a 404 HTTP error. and hence this test is cleaned up.
ebc0f98
to
33f1991
Compare
…ic behavior of Front50Service.get when an application isn't found. It doesn't return null in this case, it throws an exception. Restore previously removed "should create an application in global registries" test since it was providing useful coverage.
This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for Front50Service.
There are no behaviour changes detected with these code changes and hence no additional tests added to demonstrate the functionalities.
Few existing tests are modified to align it with the new changes.