refactor(scm/test): replace RetrofitError with SpinnakerRetrofitError…

…Handler for custom error handling in Github, GitlabCi, BitBucket, ManagedScmDelivery
spinnaker · Sep 11, 2023 · 92e8073 · 92e8073
1 parent ae0d3d5
commit 92e8073
Show file tree

Hide file tree

Showing 14 changed files with 59 additions and 39 deletions.
diff --git a/igor-web/igor-web.gradle b/igor-web/igor-web.gradle
@@ -74,6 +74,7 @@ dependencies {
     implementation "io.spinnaker.kork:kork-jedis"
     implementation "io.spinnaker.kork:kork-telemetry"
     implementation "io.spinnaker.kork:kork-plugins"
+    implementation "io.spinnaker.kork:kork-retrofit"
 
     implementation "io.github.resilience4j:resilience4j-retry"
 

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/BitBucketConfig.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/BitBucketConfig.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.igor.config
 
 import com.netflix.spinnaker.igor.scm.bitbucket.client.BitBucketClient
 import com.netflix.spinnaker.igor.scm.bitbucket.client.BitBucketMaster
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger
 import com.squareup.okhttp.Credentials
 import groovy.transform.CompileStatic
@@ -58,6 +59,7 @@ class BitBucketConfig {
       .setClient(new OkClient())
       .setConverter(new JacksonConverter())
       .setLog(new Slf4jRetrofitLogger(BitBucketClient))
+      .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
       .build()
       .create(BitBucketClient)
   }

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitHubConfig.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitHubConfig.groovy
@@ -19,6 +19,7 @@ package com.netflix.spinnaker.igor.config
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.igor.scm.github.client.GitHubClient
 import com.netflix.spinnaker.igor.scm.github.client.GitHubMaster
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
 import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger
 import groovy.transform.CompileStatic
 import groovy.util.logging.Slf4j
@@ -57,6 +58,7 @@ class GitHubConfig {
             .setClient(new OkClient())
             .setConverter(new JacksonConverter(mapper))
             .setLog(new Slf4jRetrofitLogger(GitHubClient))
+            .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
             .build()
             .create(GitHubClient)
 

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitlabCiConfig.java b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/config/GitlabCiConfig.java
@@ -21,6 +21,7 @@
 import com.netflix.spinnaker.igor.gitlabci.client.GitlabCiClient;
 import com.netflix.spinnaker.igor.gitlabci.service.GitlabCiService;
 import com.netflix.spinnaker.igor.service.BuildServices;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
 import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
 import com.squareup.okhttp.OkHttpClient;
 import java.util.Map;
@@ -95,6 +96,7 @@ public static GitlabCiClient gitlabCiClient(
         .setLog(new Slf4jRetrofitLogger(GitlabCiClient.class))
         .setLogLevel(RestAdapter.LogLevel.FULL)
         .setConverter(new JacksonConverter(objectMapper))
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .build()
         .create(GitlabCiClient.class);
   }

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/gitlabci/service/GitlabCiService.java b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/gitlabci/service/GitlabCiService.java
@@ -32,6 +32,9 @@
 import com.netflix.spinnaker.igor.travis.client.logparser.PropertyParser;
 import com.netflix.spinnaker.kork.core.RetrySupport;
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
@@ -40,7 +43,6 @@
 import java.util.stream.Collectors;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.StringUtils;
-import retrofit.RetrofitError;
 
 @Slf4j
 public class GitlabCiService implements BuildOperations, BuildProperties {
@@ -175,12 +177,12 @@ private Map<String, Object> getPropertyFileFromLog(String projectId, Integer pip
 
             return properties;
 
-          } catch (RetrofitError e) {
+          } catch (SpinnakerServerException e) {
             // retry on network issue, 404 and 5XX
-            if (e.getKind() == RetrofitError.Kind.NETWORK
-                || (e.getKind() == RetrofitError.Kind.HTTP
-                    && (e.getResponse().getStatus() == 404
-                        || e.getResponse().getStatus() >= 500))) {
+            if (e instanceof SpinnakerNetworkException
+                || (e instanceof SpinnakerHttpException
+                    && (((SpinnakerHttpException) e).getResponseCode() == 404
+                        || ((SpinnakerHttpException) e).getResponseCode() >= 500))) {
               throw e;
             }
             SpinnakerException ex = new SpinnakerException(e);

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/bitbucket/CommitController.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/bitbucket/CommitController.groovy
@@ -21,6 +21,8 @@ import com.netflix.spinnaker.igor.exceptions.UnhandledDownstreamServiceErrorExce
 import com.netflix.spinnaker.igor.scm.AbstractCommitController
 import com.netflix.spinnaker.igor.scm.bitbucket.client.BitBucketMaster
 import com.netflix.spinnaker.igor.scm.bitbucket.client.model.CompareCommitsResponse
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
@@ -29,7 +31,6 @@ import org.springframework.web.bind.annotation.RequestMapping
 import org.springframework.web.bind.annotation.RequestMethod
 import org.springframework.web.bind.annotation.RequestParam
 import org.springframework.web.bind.annotation.RestController
-import retrofit.RetrofitError
 
 @Slf4j
 @RestController(value = "BitBucketCommitController")
@@ -68,8 +69,8 @@ class CommitController extends AbstractCommitController {
       if (fromIndex > -1) {
         commitsResponse.values = commitsResponse.values.subList(0, fromIndex + 1)
       }
-    } catch (RetrofitError e) {
-      if (e.response.status == 404) {
+    } catch (SpinnakerServerException e) {
+      if (e instanceof SpinnakerHttpException && ((SpinnakerHttpException) e).getResponseCode() == 404) {
         return getNotFoundCommitsResponse(projectKey, repositorySlug, requestParams.to, requestParams.from, bitBucketMaster.baseUrl)
       }
       throw new UnhandledDownstreamServiceErrorException("Unhandled bitbucket error for ${bitBucketMaster.baseUrl}", e)

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/github/CommitController.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/github/CommitController.groovy
@@ -20,6 +20,9 @@ import com.netflix.spinnaker.igor.config.GitHubProperties
 import com.netflix.spinnaker.igor.scm.AbstractCommitController
 import com.netflix.spinnaker.igor.scm.github.client.GitHubMaster
 import com.netflix.spinnaker.igor.scm.github.client.model.CompareCommitsResponse
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
@@ -29,7 +32,6 @@ import org.springframework.web.bind.annotation.RequestMapping
 import org.springframework.web.bind.annotation.RequestMethod
 import org.springframework.web.bind.annotation.RequestParam
 import org.springframework.web.bind.annotation.RestController
-import retrofit.RetrofitError
 
 @Slf4j
 @RestController(value = "GitHubCommitController")
@@ -50,14 +52,14 @@ class CommitController extends AbstractCommitController {
 
         try {
             commitsResponse = master.gitHubClient.getCompareCommits(projectKey, repositorySlug, requestParams.to, requestParams.from)
-        } catch (RetrofitError e) {
-            if(e.getKind() == RetrofitError.Kind.NETWORK) {
-                throw new NotFoundException("Could not find the server ${master.baseUrl}")
-            } else if(e.response.status == 404) {
-                return getNotFoundCommitsResponse(projectKey, repositorySlug, requestParams.to, requestParams.from, master.baseUrl)
-            }
-            log.error("Unhandled error response, acting like commit response was not found", e)
+        } catch (SpinnakerServerException e) {
+          if(e instanceof  SpinnakerNetworkException) {
+            throw new NotFoundException("Could not find the server ${master.baseUrl}")
+          } else if(e instanceof SpinnakerHttpException && ((SpinnakerHttpException)e).getResponseCode() == 404) {
             return getNotFoundCommitsResponse(projectKey, repositorySlug, requestParams.to, requestParams.from, master.baseUrl)
+          }
+          log.error("Unhandled error response, acting like commit response was not found", e)
+          return getNotFoundCommitsResponse(projectKey, repositorySlug, requestParams.to, requestParams.from, master.baseUrl)
         }
 
         commitsResponse.commits.each {

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/github/client/GitHubMaster.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/github/client/GitHubMaster.groovy
@@ -20,9 +20,10 @@ package com.netflix.spinnaker.igor.scm.github.client
 import com.netflix.spinnaker.igor.scm.AbstractScmMaster
 import com.netflix.spinnaker.igor.scm.github.client.model.Commit
 import com.netflix.spinnaker.igor.scm.github.client.model.GetRepositoryContentResponse
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import groovy.util.logging.Slf4j
-import retrofit.RetrofitError
 
 import java.util.stream.Collectors
 
@@ -44,8 +45,8 @@ class GitHubMaster extends AbstractScmMaster {
       return response.stream()
         .map({ r -> r.path })
         .collect(Collectors.toList())
-    } catch (RetrofitError e) {
-      if (e.getKind() == RetrofitError.Kind.NETWORK) {
+    }  catch (SpinnakerServerException e) {
+      if (e instanceof  SpinnakerNetworkException) {
         throw new NotFoundException("Could not find the server ${baseUrl}")
       }
       log.error(
@@ -64,8 +65,8 @@ class GitHubMaster extends AbstractScmMaster {
         throw new NotFoundException("Unexpected content type: ${response.type}");
       }
       return new String(Base64.mimeDecoder.decode(response.content));
-    } catch (RetrofitError e) {
-      if (e.getKind() == RetrofitError.Kind.NETWORK) {
+    } catch (SpinnakerServerException e) {
+      if (e instanceof SpinnakerNetworkException) {
         throw new NotFoundException("Could not find the server ${baseUrl}")
       }
       log.error(

diff --git a/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitController.java b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitController.java
@@ -20,6 +20,9 @@
 import com.netflix.spinnaker.igor.scm.AbstractCommitController;
 import com.netflix.spinnaker.igor.scm.gitlab.client.GitLabMaster;
 import com.netflix.spinnaker.igor.scm.gitlab.client.model.CompareCommitsResponse;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
 import java.util.HashMap;
 import java.util.List;
@@ -30,7 +33,6 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.web.bind.annotation.*;
-import retrofit.RetrofitError;
 
 @RestController(value = "GitLabCommitController")
 @ConditionalOnProperty("gitlab.base-url")
@@ -66,10 +68,11 @@ public List<Map<String, Object>> compareCommits(
       queryMap.put("from", fromParam);
       commitsResponse =
           gitLabMaster.getGitLabClient().getCompareCommits(projectKey, repositorySlug, queryMap);
-    } catch (RetrofitError e) {
-      if (e.getKind() == RetrofitError.Kind.NETWORK) {
+    } catch (SpinnakerServerException e) {
+      if (e instanceof SpinnakerNetworkException) {
         throw new NotFoundException("Could not find the server " + gitLabMaster.getBaseUrl());
-      } else if (e.getResponse().getStatus() == 404) {
+      } else if (e instanceof SpinnakerHttpException
+          && ((SpinnakerHttpException) e).getResponseCode() == 404) {
         return getNotFoundCommitsResponse(
             projectKey, repositorySlug, toParam, fromParam, gitLabMaster.getBaseUrl());
       }

diff --git a/igor-web/src/main/java/com/netflix/spinnaker/igor/scm/ManagedDeliveryScmController.java b/igor-web/src/main/java/com/netflix/spinnaker/igor/scm/ManagedDeliveryScmController.java
@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.igor.scm;
 
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -26,7 +28,6 @@
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
-import retrofit.RetrofitError;
 
 /** Exposes APIs to retrieve Managed Delivery declarative manifests from source control repos. */
 @RestController
@@ -85,13 +86,13 @@ public ResponseEntity<Map<String, Object>> getDeliveryConfigManifest(
       Object errorDetails = e.getMessage();
       if (e instanceof IllegalArgumentException) {
         status = HttpStatus.BAD_REQUEST;
-      } else if (e instanceof RetrofitError) {
-        RetrofitError re = (RetrofitError) e;
-        if (re.getKind() == RetrofitError.Kind.HTTP) {
-          status = HttpStatus.valueOf(re.getResponse().getStatus());
-          errorDetails = re.getBodyAs(Map.class);
+      } else if (e instanceof SpinnakerServerException) {
+        if (e instanceof SpinnakerHttpException) {
+          SpinnakerHttpException re = (SpinnakerHttpException) e;
+          status = HttpStatus.valueOf(re.getResponseCode());
+          errorDetails = re.getResponseBody();
         } else {
-          errorDetails = "Error calling downstream system: " + re.getMessage();
+          errorDetails = "Error calling downstream system: " + e.getMessage();
         }
       }
       return buildErrorResponse(status, errorDetails);

diff --git a/...eb/src/test/groovy/com/netflix/spinnaker/igor/scm/ManagedDeliveryScmControllerSpec.groovy b/...eb/src/test/groovy/com/netflix/spinnaker/igor/scm/ManagedDeliveryScmControllerSpec.groovy
@@ -16,7 +16,7 @@
 
 package com.netflix.spinnaker.igor.scm
 
-import org.springframework.boot.test.json.JacksonTester
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import org.springframework.http.HttpStatus
 import org.springframework.http.ResponseEntity
 import retrofit.RetrofitError
@@ -106,13 +106,13 @@ class ManagedDeliveryScmControllerSpec extends Specification {
   void '404 error from service is propagated'() {
     given:
     1 * service.getDeliveryConfigManifest(scmType, project, repo, dir, manifest, ref) >> {
-      throw new RetrofitError("oops!", "http://nada",
+      throw new SpinnakerHttpException(new RetrofitError("oops!", "http://nada",
         new Response("http://nada", 404, "", [], new TypedString('{"detail": "oops!"}')),
         new JacksonConverter(),
         null,
         RetrofitError.Kind.HTTP,
         null
-      )
+      ))
     }
 
     when:

diff --git a/...-web/src/test/groovy/com/netflix/spinnaker/igor/scm/bitbucket/CommitControllerSpec.groovy b/...-web/src/test/groovy/com/netflix/spinnaker/igor/scm/bitbucket/CommitControllerSpec.groovy
@@ -24,6 +24,7 @@ import com.netflix.spinnaker.igor.scm.bitbucket.client.model.Author
 import com.netflix.spinnaker.igor.scm.bitbucket.client.model.Commit
 import com.netflix.spinnaker.igor.scm.bitbucket.client.model.CompareCommitsResponse
 import com.netflix.spinnaker.igor.scm.bitbucket.client.model.User
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import retrofit.RetrofitError
 import retrofit.client.Response
 import spock.lang.Specification
@@ -64,7 +65,7 @@ class CommitControllerSpec extends Specification {
 
   void 'get 404 from bitBucketClient and return one commit'() {
     when:
-    1 * client.getCompareCommits(projectKey, repositorySlug, clientParams) >> {throw new RetrofitError(null, null, new Response("http://foo.com", 404, "test reason", [], null), null, null, null, null)}
+    1 * client.getCompareCommits(projectKey, repositorySlug, clientParams) >> {throw new SpinnakerHttpException(new RetrofitError(null, null, new Response("http://foo.com", 404, "test reason", [], null), null, null, null, null))}
     def result = controller.compareCommits(projectKey, repositorySlug, controllerParams)
 
     then:

diff --git a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/scm/github/CommitControllerSpec.groovy b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/scm/github/CommitControllerSpec.groovy
@@ -24,6 +24,7 @@ import com.netflix.spinnaker.igor.scm.github.client.model.Author
 import com.netflix.spinnaker.igor.scm.github.client.model.Commit
 import com.netflix.spinnaker.igor.scm.github.client.model.CommitInfo
 import com.netflix.spinnaker.igor.scm.github.client.model.CompareCommitsResponse
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import retrofit.RetrofitError
 import retrofit.client.Response
 import spock.lang.Specification
@@ -65,7 +66,7 @@ class CommitControllerSpec extends Specification {
 
     void 'get 404 from client and return one commit'() {
         when:
-        1 * client.getCompareCommits(projectKey, repositorySlug, queryParams.to, queryParams.from) >> {throw new RetrofitError(null, null, new Response("http://foo.com", 404, "test reason", [], null), null, null, null, null)}
+        1 * client.getCompareCommits(projectKey, repositorySlug, queryParams.to, queryParams.from) >> {throw new SpinnakerHttpException(new RetrofitError(null, null, new Response("http://foo.com", 404, "test reason", [], null), null, null, null, null))}
         def result = controller.compareCommits(projectKey, repositorySlug, queryParams)
 
         then:

diff --git a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitControllerSpec.groovy b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/scm/gitlab/CommitControllerSpec.groovy
@@ -22,6 +22,7 @@ import com.netflix.spinnaker.igor.scm.gitlab.client.GitLabClient
 import com.netflix.spinnaker.igor.scm.gitlab.client.GitLabMaster
 import com.netflix.spinnaker.igor.scm.gitlab.client.model.Commit
 import com.netflix.spinnaker.igor.scm.gitlab.client.model.CompareCommitsResponse
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
 import retrofit.RetrofitError
 import retrofit.client.Response
 import spock.lang.Specification
@@ -67,7 +68,7 @@ class CommitControllerSpec extends Specification {
     void 'get 404 from client and return one commit'() {
         when:
         1 * client.getCompareCommits(projectKey, repositorySlug, [from: queryParams.to, to: queryParams.from]) >> {
-            throw new RetrofitError(null, null, new Response("http://foo.com", 404, "test reason", [], null), null, null, null, null)
+            throw new SpinnakerHttpException(new RetrofitError(null, null, new Response("http://foo.com", 404, "test reason", [], null), null, null, null, null))
         }
         def result = controller.compareCommits(projectKey, repositorySlug, queryParams)