refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service #4623
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
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
2 times, most recently
from
cfc1b7b
to
328d7b8
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
2 times, most recently
from
7fc719e
to
483b3e0
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
483b3e0
to
4d0485f
Compare
Pranav-b-7
changed the title
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
8 times, most recently
from
ee64ec5
to
d81480a
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
3459464
to
fcc2e80
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
fcc2e80
to
29643d8
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
4c6aa53
to
886e2b1
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
9 times, most recently
from
13169ef
to
ae24d3f
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
2 times, most recently
from
5ac2f60
to
59945dd
Compare
dbyron-sf
reviewed
Apr 6, 2024
orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy
Show resolved
Hide resolved
dbyron-sf
reviewed
Apr 6, 2024
.../src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/job/WaitOnJobCompletion.groovy
Outdated
Show resolved
Hide resolved
dbyron-sf
reviewed
Apr 6, 2024
orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/job/JobUtils.java
Outdated
Show resolved
Hide resolved
dbyron-sf
reviewed
Apr 6, 2024
| @@ -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.
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.
Looks like a similar thing happens in UpsertApplicationTaskSpec ...
There was a problem hiding this comment.
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.
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.
dbyron-sf
reviewed
Apr 6, 2024
| @@ -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.
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.
The point of this PR is to move to Spinnaker*Exception, so let's leave the logic as it is for now.
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
59945dd
to
d02575e
Compare
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
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.
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
81dcef8
to
ebc0f98
Compare
added 4 commits
April 16, 2024 12:23
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.
Pranav-b-7
force-pushed
the
remove-retrofitError-Front50Service
branch
from
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.
dbyron-sf
approved these changes
Apr 16, 2024
dbyron-sf
added
ready to merge
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.
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.