refactor(retrofit2): upgrade code to use retrofit2 #1313

alice485 · 2025-03-12T15:33:10Z

This PR upgrades all code to use retrofit2 and the retrofit2 boilerplate code implemented in kork-retrofit
Removes all dependencies to retrofit1
Removes all references to retrofit1 and RetrofitError

dbyron-sf · 2025-03-12T15:44:36Z

@Mergifyio update

mergify · 2025-03-12T15:44:45Z

update

✅ Branch has been successfully updated

igor-web/src/main/java/com/netflix/spinnaker/igor/config/HelmConverterFactory.java

igor-monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/config/TravisConfig.java

igor-web/src/main/groovy/com/netflix/spinnaker/igor/build/BuildController.groovy

dbyron-sf · 2025-03-12T16:00:42Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/JenkinsConfig.groovy

-        OkHttpClient.Builder clientBuilder = client.newBuilder().readTimeout(timeout, TimeUnit.MILLISECONDS)
+        Interceptor requestInterceptor = (jenkinsRetrofitRequestInterceptorProvider != null) ? jenkinsRetrofitRequestInterceptorProvider.provide(host): null
+        OkHttpClient.Builder clientBuilder = okHttpClientConfig.createForRetrofit2().readTimeout(timeout, TimeUnit.MILLISECONDS)
+        if(requestInterceptor != null){


Suggested change

if(requestInterceptor != null){

if (requestInterceptor != null) {

dbyron-sf · 2025-03-12T16:04:19Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/service/JenkinsService.java

          } catch (SpinnakerHttpException e) {
            if (e.getResponseCode() == 404 || e.getResponseCode() >= 500) {
+              e.setRetryable(true); // 404 not retryable by default


Thanks for catching this. Is there a test for this code path?

dbyron-sf · 2025-03-12T16:20:07Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/gitlabci/service/GitlabCiService.java

@@ -182,6 +190,7 @@ private Map<String, Object> getPropertyFileFromLog(String projectId, Long pipeli
          } catch (SpinnakerHttpException e) {
            // retry on 404 and 5XX
            if (e.getResponseCode() == 404 || e.getResponseCode() >= 500) {
+              e.setRetryable(true); // 404 not retryable by default


Is there a test for this?

dbyron-sf · 2025-03-12T16:24:19Z

igor-web/src/main/java/com/netflix/spinnaker/igor/concourse/service/ConcourseService.java

-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
+import java.util.*;


please avoid star imports

dbyron-sf · 2025-03-12T16:25:12Z

igor-web/src/main/java/com/netflix/spinnaker/igor/concourse/service/ConcourseService.java

-        .values()
-        .stream()
-        .collect(Collectors.toList());
+    return new ArrayList<>(


Why not keep the .collect(Collectors.toList()); ?

dbyron-sf · 2025-03-12T16:27:41Z

igor-web/src/test/groovy/com/netflix/spinnaker/igor/concourse/client/ConcourseClientSpec.groovy

@@ -54,8 +57,6 @@ class ConcourseClientSpec extends Specification {

        req3.path == '/sky/userinfo'
        req3.getHeader('Authorization') == "bearer my_token"
-
-        resp.status == 200


What can we do to keep this assertion? Is it a change to use Retrofit2SyncCall.executeCall instead of execute?

yeah, I was able to put the assertion back after changing the method call to Retrofit2syncCall.executeCall

dbyron-sf · 2025-03-12T16:31:31Z

igor-web/src/main/java/com/netflix/spinnaker/igor/config/HelmConverterFactory.java

+      Type type, Annotation[] annotations, Retrofit retrofit) {
+    // If the return type is a String, provide it as such
+    if (type.getTypeName().equals("java.lang.String")) {
+      return (Converter<ResponseBody, String>) value -> IOUtils.toString(value.byteStream());


The old code catches IOException and turns it into ConversionException. Is it more correct to throw SpinnakerConversion in that case here?

dbyron-sf · 2025-03-12T16:34:21Z

gradle.properties

@@ -1,4 +1,4 @@
-fiatVersion=1.53.0


I asked mergify to update, but it'd be cleaner for you to rebase and make this commit disappear.

removed the commit

github is still saying that this branch is out of date. Can you try rebasing again please?

kirangodishala · 2025-03-12T18:44:10Z

igor-monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/config/TravisConfig.java

-        .setLog(new Slf4jRetrofitLogger(TravisClient.class))
-        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
+    return new Retrofit.Builder()
+        .baseUrl(address)


Can you refer this and make sure the address doesn't end up in baseUrl must end in / error. And this is applicable to all such retrofit2 client definitions.

kirangodishala · 2025-03-12T19:04:08Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/service/JenkinsService.java

@@ -264,7 +271,8 @@ public void updateBuild(String jobName, Long buildNumber, UpdatedBuild updatedBu



Retrofit2SyncCall.execute() is missing for jenkinsClient.submitDescription() at line 267

kirangodishala · 2025-03-12T19:19:40Z

igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/BitBucketConfig.groovy

-      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
+  BitBucketClient bitBucketClient(String address, String username, String password, OkHttp3ClientConfiguration okHttpClientConfig) {
+    new Retrofit.Builder()
+      .baseUrl(address)


Make sure the baseUrl doesn't cause baseUrl must end in /. Refer this for more details.

used the PR you linked as a reference to fix this and added some tests. Added all the missing Retrofit2SyncCall.execute() calls also.

kirangodishala

@alice485 - there are several places where Retrofit2SynCall.execute() is missing. Here are what I found so far:

PluginsBuildMonitor.java

TravisBuildMonitor.java

DockerMonitor.groovy

GitlabCiBuildMonitor.java

JenkinsBuildMonitor.groovy

ConcourseBuildMonitor.java

EchoServiceHealthIndicator.java

HelmMonitor.java

NexusEventPoster.java

…ofit2

…y class to prevent 'baseUrl must end in /' error

dbyron-sf · 2025-03-13T21:18:11Z

igor-web/src/main/java/com/netflix/spinnaker/igor/concourse/client/ConcourseClient.java

-  public Response userInfo() {
-    return skyServiceV1 != null ? skyServiceV1.userInfo() : skyServiceV2.userInfo();
+  public Response<ResponseBody> userInfo() {
+    return skyServiceV1 != null


a nit, but

Suggested change

return skyServiceV1 != null

return Retrofit2SyncCall.executeCall(skyServiceV1 != null ? skyServiceV1.userInfo() : skyServiceV2.userInfo());

alice garcia added 12 commits March 11, 2025 16:27

refactor(retrofit2): upgrade igor-core package to retrofit2

2c331bd

refactor(retrofit2): upgrade igor-monitor-artifactory to retrofit2

71b6ffe

refactor(retrofit2): upgrade igor-monitor-plugins to retrofit2

ef4f156

refactor(retrofit2): upgrade igor-monitor-travis to retrofit2

f2c62b9

refactor(retrofit2): upgrade igor-web jenkins code to retrofit2

79ed6b7

refactor(retrofit2): upgrade igor-web bitbucket code to retrofit2

594511c

refactor(retrofit2): upgrade igor-web github code to retrofit2

801a351

refactor(retrofit2): upgrade igor-web gitlab code to retrofit2

2efb1b2

refactor(retrofit2): upgrade igor-web stash code to retrofit2

2f57bff

refactor(retrofit2): upgrade igor-web wercker code to retrofit2

4fe5dbc

refactor(retrofit2): upgrade igor-web concourse code to retrofit2

100d5bc

refactor(retrofit2): upgrade igor-web helm code to retrofit2

7e89722

dbyron-sf reviewed Mar 12, 2025

View reviewed changes

igor-web/src/main/java/com/netflix/spinnaker/igor/config/HelmConverterFactory.java Show resolved Hide resolved

dbyron-sf reviewed Mar 12, 2025

View reviewed changes

igor-monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/config/TravisConfig.java Outdated Show resolved Hide resolved

dbyron-sf reviewed Mar 12, 2025

View reviewed changes

igor-web/src/main/groovy/com/netflix/spinnaker/igor/build/BuildController.groovy Show resolved Hide resolved

dbyron-sf reviewed Mar 12, 2025

View reviewed changes

kirangodishala reviewed Mar 12, 2025

View reviewed changes

kirangodishala requested changes Mar 13, 2025

View reviewed changes

refactor(retrofit2): upgrade remaining misc files in igor-web to retr…

98b4485

…ofit2

alice garcia added 2 commits March 13, 2025 16:23

refactor(retrofit2): add missing Retrofit2SyncCall.execute() calls

d39a1ac

refactor(retrofit2): remove leading slashes in endpoints + add utilit…

0c4d482

…y class to prevent 'baseUrl must end in /' error

alice485 force-pushed the upgrade-to-retrofit2 branch from 6d69e8a to 0c4d482 Compare March 13, 2025 20:24

dbyron-sf reviewed Mar 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(retrofit2): upgrade code to use retrofit2 #1313

refactor(retrofit2): upgrade code to use retrofit2 #1313

alice485 commented Mar 12, 2025

dbyron-sf commented Mar 12, 2025

mergify bot commented Mar 12, 2025

dbyron-sf Mar 12, 2025

alice485 Mar 13, 2025

dbyron-sf Mar 12, 2025

dbyron-sf Mar 12, 2025

dbyron-sf Mar 12, 2025

alice485 Mar 13, 2025

dbyron-sf Mar 12, 2025

dbyron-sf Mar 12, 2025

alice485 Mar 13, 2025

dbyron-sf Mar 12, 2025

dbyron-sf Mar 12, 2025

alice485 Mar 13, 2025

dbyron-sf Mar 13, 2025

kirangodishala Mar 12, 2025

kirangodishala Mar 12, 2025

kirangodishala Mar 12, 2025

alice485 Mar 13, 2025

kirangodishala left a comment

dbyron-sf Mar 13, 2025

	if(requestInterceptor != null){
	if (requestInterceptor != null) {

		@@ -264,7 +271,8 @@ public void updateBuild(String jobName, Long buildNumber, UpdatedBuild updatedBu

	return skyServiceV1 != null
	return Retrofit2SyncCall.executeCall(skyServiceV1 != null ? skyServiceV1.userInfo() : skyServiceV2.userInfo());

refactor(retrofit2): upgrade code to use retrofit2 #1313

Are you sure you want to change the base?

refactor(retrofit2): upgrade code to use retrofit2 #1313

Conversation

alice485 commented Mar 12, 2025

dbyron-sf commented Mar 12, 2025

mergify bot commented Mar 12, 2025

✅ Branch has been successfully updated

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirangodishala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment