refactor(Githubclient): make changes to Git(master|lab) and BitBucket… (

#1104) * refactor(scm/test): replace RetrofitError with SpinnakerRetrofitErrorHandler for custom error handling in Github, GitlabCi, BitBucket, ManagedScmDelivery * refactor(scm/test): separated SpinnakerNetworkException and SpinnakerHttpException exceptions for readability * fix(scm): restore original behavior in GitHubCommitController, fix compile error in ManagedDeliveryScmController * fix(scm): restore original behavior in GitHubCommitController, restored missing line for GitHubCommitController in catch of SpinnakerServerException * fix(GitHubMaster,GitLabCommitController,ManagedDeliveryScmController): change Spinnaker*Exception handling code for readability * refactor(web): add explicit SpinnakerNetworkException catch block in scm/github/CommitController.compareCommits to clean up the code / reduce instanceof checks * Update igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/github/client/GitHubMaster.groovy --------- Co-authored-by: SheetalAtre <[email protected]> Co-authored-by: David Byron <[email protected]> Co-authored-by: David Byron <[email protected]>
spinnaker · Sep 19, 2023 · c48d215 · c48d215
1 parent 03c1ace
commit c48d215
Show file tree

Hide file tree

Showing 13 changed files with 73 additions and 53 deletions.
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
@@ -31,7 +31,9 @@
 import com.netflix.spinnaker.igor.service.BuildProperties;
 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 +42,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,17 +176,20 @@ private Map<String, Object> getPropertyFileFromLog(String projectId, Integer pip
 
             return properties;
 
-          } catch (RetrofitError 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))) {
+          } catch (SpinnakerNetworkException e) {
+            // retry on network issue
+            throw e;
+          } catch (SpinnakerHttpException e) {
+            // retry on 404 and 5XX
+            if (e.getResponseCode() == 404 || e.getResponseCode() >= 500) {
               throw e;
             }
-            SpinnakerException ex = new SpinnakerException(e);
-            ex.setRetryable(false);
-            throw ex;
+            e.setRetryable(false);
+            throw e;
+          } catch (SpinnakerServerException e) {
+            // do not retry
+            e.setRetryable(false);
+            throw e;
           } catch (IOException e) {
             log.error("Error while parsing GitLab CI log to build properties", e);
             return properties;

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 (SpinnakerNetworkException e) {
+            throw new NotFoundException("Could not find the server ${master.baseUrl}")
+        } catch (SpinnakerServerException e) {
+          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,10 +45,10 @@ class GitHubMaster extends AbstractScmMaster {
       return response.stream()
         .map({ r -> r.path })
         .collect(Collectors.toList())
-    } catch (RetrofitError e) {
-      if (e.getKind() == RetrofitError.Kind.NETWORK) {
-        throw new NotFoundException("Could not find the server ${baseUrl}")
-      }
+    } catch (SpinnakerNetworkException e) {
+      throw new NotFoundException("Could not find the server ${baseUrl}")
+    }
+    catch (SpinnakerServerException e) {
       log.error(
         "Failed to fetch file from {}/{}/{}, reason: {}",
         projectKey, repositorySlug, path, e.message
@@ -64,10 +65,9 @@ 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) {
-        throw new NotFoundException("Could not find the server ${baseUrl}")
-      }
+    } catch (SpinnakerNetworkException e) {
+      throw new NotFoundException("Could not find the server ${baseUrl}")
+    }catch (SpinnakerServerException e) {
       log.error(
         "Failed to fetch file from {}/{}/{}, reason: {}",
         projectKey, repositorySlug, path, e.message

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,13 +68,15 @@ 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) {
-        throw new NotFoundException("Could not find the server " + gitLabMaster.getBaseUrl());
-      } else if (e.getResponse().getStatus() == 404) {
-        return getNotFoundCommitsResponse(
-            projectKey, repositorySlug, toParam, fromParam, gitLabMaster.getBaseUrl());
+    } catch (SpinnakerNetworkException e) {
+      throw new NotFoundException("Could not find the server " + gitLabMaster.getBaseUrl());
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() != 404) {
+        log.error("Unhandled error response, acting like commit response was not found", e);
       }
+      return getNotFoundCommitsResponse(
+          projectKey, repositorySlug, toParam, fromParam, gitLabMaster.getBaseUrl());
+    } catch (SpinnakerServerException e) {
       log.error("Unhandled error response, acting like commit response was not found", e);
       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,15 +86,14 @@ 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 {
-          errorDetails = "Error calling downstream system: " + re.getMessage();
-        }
+      } else if (e instanceof SpinnakerHttpException) {
+        SpinnakerHttpException spinnakerHttpException = (SpinnakerHttpException) e;
+        status = HttpStatus.valueOf(spinnakerHttpException.getResponseCode());
+        errorDetails = spinnakerHttpException.getResponseBody();
+      } else if (e instanceof SpinnakerServerException) {
+        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)