Skip to content
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

feat(retrofit2): add ErrorHandlingExecutorCallAdapterFactory to the retrofit2 client #1225

Merged

Conversation

kirangodishala
Copy link
Contributor

  • Retrofit2 client defined in Retrofit2ServiceFactory doesn't have CallAdapterFactory set.
  • This PR sets ErrorHandlingExecutorCallAdapterFactory to the retrofit2 client which is responsible for classifying the exceptions into SpinnakerHttpException, SpinnakerNetworkException or SpinnakerServerException.

@@ -150,6 +154,54 @@ void testRetrofit2Client_withInterceptor() {
.withHeader("Authorization", equalTo("Bearer my-token")));
}

@Test
void testRetrofit2Client_withHttpException() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Side question...is testRetrofit2ClientWithResponse the same as testRetrofit2Client? If so, can you remove one please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a couple of assertions now to distinguish the two tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They still seem quite similar, but thanks for making them at least a bit different.

@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Feb 7, 2025
@mergify mergify bot added the auto merged label Feb 7, 2025
@mergify mergify bot merged commit 4e1a797 into spinnaker:master Feb 7, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged ready to merge Approved and ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants