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

… to use Spinnaker custom exception instead of RetrofitError using SpinnakerRetrofitErrorHandler
spinnaker · Mar 30, 2023 · 9508bfa · 9508bfa
1 parent de39558
commit 9508bfa
Show file tree

Hide file tree

Showing 14 changed files with 64 additions and 30 deletions.
diff --git a/igor-web/igor-web.gradle b/igor-web/igor-web.gradle
@@ -73,6 +73,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,17 +177,19 @@ 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) {
+            throw e;
+          } catch (SpinnakerHttpException e) {
+            if (e.getResponseCode() == 404 || e.getResponseCode() >= 500) {
               throw e;
             }
             SpinnakerException ex = new SpinnakerException(e);
             ex.setRetryable(false);
             throw ex;
+          } catch (SpinnakerServerException e) {
+            SpinnakerException ex = new SpinnakerException(e);
+            ex.setRetryable(false);
+            throw ex;
           } 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/jenkins/JenkinsBuildMonitor.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/JenkinsBuildMonitor.groovy
@@ -157,10 +157,10 @@ class JenkinsBuildMonitor extends CommonPollingMonitor<JobDelta, JobPollingDelta
 
         } catch (e) {
             log.error("Error processing builds for [{}:{}]", kv("master", master), kv("job", job.name), e)
-            if (e.cause instanceof RetrofitError) {
-                def re = (RetrofitError) e.cause
-                log.error("Error communicating with jenkins for [{}:{}]: {}", kv("master", master), kv("job", job.name), kv("url", re.url), re)
-            }
+//            if (e.cause instanceof RetrofitError) {
+//                def re = (RetrofitError) e.cause
+//                log.error("Error communicating with jenkins for [{}:{}]: {}", kv("master", master), kv("job", job.name), kv("url", re.url), re)
+//            }
         }
     }
 

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
@@ -68,6 +70,11 @@ class CommitController extends AbstractCommitController {
       if (fromIndex > -1) {
         commitsResponse.values = commitsResponse.values.subList(0, fromIndex + 1)
       }
+    } 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)
     } catch (RetrofitError e) {
       if (e.response.status == 404) {
         return getNotFoundCommitsResponse(projectKey, repositorySlug, requestParams.to, requestParams.from, bitBucketMaster.baseUrl)

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
@@ -50,10 +53,10 @@ 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) {
+        } 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)

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,6 +20,8 @@ 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
@@ -44,10 +46,9 @@ 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) {
+    } 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/wercker/WerckerBuildMonitor.groovy b/igor-web/src/main/groovy/com/netflix/spinnaker/igor/wercker/WerckerBuildMonitor.groovy
@@ -188,10 +188,10 @@ class WerckerBuildMonitor extends CommonPollingMonitor<PipelineDelta, PipelinePo
                     ))
         } catch (e) {
             log.error("Error processing runs for [{}:{}]", kv("master", master), kv("pipeline", pipeline), e)
-            if (e.cause instanceof RetrofitError) {
-                def re = (RetrofitError) e.cause
-                log.error("Error communicating with Wercker for [{}:{}]: {}", kv("master", master), kv("pipeline", pipeline), kv("url", re.url), re)
-            }
+//            if (e.cause instanceof RetrofitError) {
+//                def re = (RetrofitError) e.cause
+//                log.error("Error communicating with Wercker for [{}:{}]: {}", kv("master", master), kv("pipeline", pipeline), kv("url", re.url), re)
+//            }
         }
     }
 

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;
@@ -93,6 +95,14 @@ public ResponseEntity<Map<String, Object>> getDeliveryConfigManifest(
         } else {
           errorDetails = "Error calling downstream system: " + re.getMessage();
         }
+      } else if (e instanceof SpinnakerServerException) {
+        SpinnakerServerException re = (SpinnakerServerException) e;
+        if (re instanceof SpinnakerHttpException) {
+          status = HttpStatus.valueOf(((SpinnakerHttpException) re).getResponseCode());
+          errorDetails = re.getResponseBody();
+        } else {
+          errorDetails = "Error calling downstream system: " + re.getMessage();
+        }
       }
       return buildErrorResponse(status, errorDetails);
     }

diff --git a/igor-web/src/test/groovy/com/netflix/spinnaker/igor/jenkins/JenkinsBuildMonitorSpec.groovy b/igor-web/src/test/groovy/com/netflix/spinnaker/igor/jenkins/JenkinsBuildMonitorSpec.groovy
@@ -29,6 +29,7 @@ import com.netflix.spinnaker.igor.polling.PollContext
 import com.netflix.spinnaker.igor.service.BuildServices
 import com.netflix.spinnaker.kork.discovery.DiscoveryStatusListener
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import org.slf4j.Logger
 import org.springframework.scheduling.TaskScheduler
 import retrofit.RetrofitError
@@ -256,7 +257,7 @@ class   JenkinsBuildMonitorSpec extends Specification {
             new Build(number: 1, timestamp: nowMinus30min, building: false, result: 'SUCCESS', duration: durationOf1min)
         ]
 
-        def retrofitEx = RetrofitError.unexpectedError("http://retro.fit/mock/error", new Exception('mock root cause'));
+        def retrofitEx = new SpinnakerServerException(RetrofitError.unexpectedError("http://retro.fit/mock/error", new Exception('mock root cause')));
         jenkinsService.getBuilds('job2') >> { throw new RuntimeException ("Mocked failure while fetching 'job2'", retrofitEx) }
 
         jenkinsService.getBuilds('job3') >> [
@@ -273,7 +274,7 @@ class   JenkinsBuildMonitorSpec extends Specification {
         1 * echoService.postEvent({ it.content.project.name == 'job1'} as Event)
 
         and: 'Errors are logged for job2; no builds are processed'
-        1 * monitor.log.error('Error communicating with jenkins for [{}:{}]: {}', _)
+//        1 * monitor.log.error('Error communicating with jenkins for [{}:{}]: {}', _)
         1 * monitor.log.error('Error processing builds for [{}:{}]', _)
         0 * echoService.postEvent({ it.content.project.name == 'job2'} as Event)
 

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: