-
Notifications
You must be signed in to change notification settings - Fork 653
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(retrofit2): replace retrofit client with retrofit2 client #1466
Conversation
kirangodishala
commented
Dec 29, 2024
- This PR makes use of retrofit 2.x client in place of retrofit 1.9 and make required changes accordingly.
- Replaced all RetrofitError references with SpinnakerServerException and/or its sub classes.
- All the retrofit1 dependencies and references are removed.
- Retrofit2 boilerplate code was already implemented in kork's kork-retrofit module and this PR makes use of it.
We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:
See our server-side conventions here. |
ee81fce
to
28ace69
Compare
...ifications/src/main/groovy/com/netflix/spinnaker/echo/cdevents/CDEventsConverterFactory.java
Outdated
Show resolved
Hide resolved
...tions/src/main/groovy/com/netflix/spinnaker/echo/notification/CDEventsNotificationAgent.java
Outdated
Show resolved
Hide resolved
echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/config/GoogleChatConfig.groovy
Outdated
Show resolved
Hide resolved
echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/config/GoogleChatConfig.groovy
Outdated
Show resolved
Hide resolved
echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/googlechat/GoogleChatService.java
Outdated
Show resolved
Hide resolved
.create(GithubService.class); | ||
|
||
return githubClient; | ||
return new Retrofit.Builder() |
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 above code doesn't have:
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
so...is there still some RetrofitError-based exception handling? Or maybe there isn't, and we're getting an improvement by moving to ErrorHandlingExecutorCallAdapterFactory....and that improvement is...SpinnakerServerException and children in theory provide better error messages than RetrofitError.
echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/github/GithubService.java
Outdated
Show resolved
Hide resolved
...cations/src/main/groovy/com/netflix/spinnaker/echo/notification/GithubNotificationAgent.java
Outdated
Show resolved
Hide resolved
def slackClient = new RestAdapter.Builder() | ||
.setEndpoint(slackEndpoint) | ||
.setConverter(new JacksonConverter()) | ||
.setClient(retrofitClient) |
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'll stop commenting, but...another place with no SpinnakerRetrofitErrorHandler.
...tifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy
Outdated
Show resolved
Hide resolved
...tifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy
Outdated
Show resolved
Hide resolved
echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/config/PagerDutyConfig.java
Outdated
Show resolved
Hide resolved
...ons/src/main/groovy/com/netflix/spinnaker/echo/pagerduty/PagerDutyNotificationService.groovy
Outdated
Show resolved
Hide resolved
...cations/src/main/groovy/com/netflix/spinnaker/echo/microsoftteams/MicrosoftTeamsService.java
Show resolved
Hide resolved
...src/main/groovy/com/netflix/spinnaker/echo/notification/MicrosoftTeamsNotificationAgent.java
Outdated
Show resolved
Hide resolved
echo-telemetry/src/main/java/com/netflix/spinnaker/echo/config/TelemetryConfig.kt
Outdated
Show resolved
Hide resolved
...telemetry/src/test/kotlin/com/netflix/spinnaker/echo/telemetry/TelemetryEventListenerTest.kt
Outdated
Show resolved
Hide resolved
...telemetry/src/test/kotlin/com/netflix/spinnaker/echo/telemetry/TelemetryEventListenerTest.kt
Outdated
Show resolved
Hide resolved
.../src/main/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationService.groovy
Outdated
Show resolved
Hide resolved
e30597f
to
f6dac52
Compare
…va/.. in echo-notifications
…it2's encoded=true
75c6d1d
to
3bb2230
Compare
...fications/src/main/java/com/netflix/spinnaker/echo/notification/GithubNotificationAgent.java
Show resolved
Hide resolved
echo-notifications/src/main/java/com/netflix/spinnaker/echo/config/PagerDutyConfig.java
Outdated
Show resolved
Hide resolved
3bb2230
to
1937a41
Compare
...tifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy
Outdated
Show resolved
Hide resolved
...tifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy
Outdated
Show resolved
Hide resolved
1937a41
to
ace41e5
Compare