fix(docker): address review comments on the fix to the issue of retrofit2 replacing ? with %3F causing api failure with 400

Author: kirangodishala

clouddriver-docker/src/main/groovy/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/auth/DockerBearerTokenService.groovy

@@ -19,6 +19,7 @@ package com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.DockerUserAgent
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.exception.DockerRegistryAuthenticationException
 import com.netflix.spinnaker.config.DefaultServiceEndpoint
+import com.netflix.spinnaker.kork.annotations.VisibleForTesting
 import com.netflix.spinnaker.kork.client.ServiceClientProvider
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall
 import groovy.util.logging.Slf4j
@@ -236,7 +237,8 @@ class DockerBearerTokenService {
     cachedTokens.remove(repository)
   }
 
-  private interface TokenService {
+  @VisibleForTesting
+  interface TokenService {
     @GET("/{path}")
     @Headers([
       "Docker-Distribution-API-Version: registry/2.0"

clouddriver-docker/src/main/groovy/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/client/DockerRegistryClient.groovy

@@ -474,7 +474,12 @@ class DockerRegistryClient {
     return tags
   }
 
-  static def parseForQueryParams(String nextPath) {
+  /**
+   * This method takes a string that might contain a query string and splits it into the path and the query parameters.
+   * @param nextPath the string that might contain a query string
+   * @return a tuple containing the path (without query string) and a map of query parameters
+   */
+  static Tuple2<String, Map<String, String>> parseForQueryParams(String nextPath) {
     def nextPathNew
     def queryParamsString
     Map<String, String> queryParams = [:]

clouddriver-docker/src/test/java/com/netflix/spinnaker/clouddriver/docker/registry/api/v2/client/DockerRegistryServiceTest.java

@@ -16,9 +16,11 @@
 
 package com.netflix.spinnaker.clouddriver.docker.registry.api.v2.client;
 
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
 import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth.DockerBearerToken;
+import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.auth.DockerBearerTokenService;
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
 import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
 import java.io.IOException;
@@ -29,20 +31,14 @@
 import okhttp3.ResponseBody;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
-import retrofit2.Call;
 import retrofit2.Response;
 import retrofit2.Retrofit;
 import retrofit2.converter.jackson.JacksonConverterFactory;
-import retrofit2.http.GET;
-import retrofit2.http.Header;
-import retrofit2.http.Headers;
-import retrofit2.http.Path;
-import retrofit2.http.Query;
 
 public class DockerRegistryServiceTest {
 
   static DockerRegistryClient.DockerRegistryService dockerRegistryService;
-  static TokenService tokenService;
+  static DockerBearerTokenService.TokenService tokenService;
   String service = "registry.docker.io";
   String scope = "repository:library/nginx:pull";
   String repository = "library/nginx";
@@ -53,7 +49,8 @@ public class DockerRegistryServiceTest {
 
   @BeforeAll
   public static void setup() {
-    tokenService = buildService(TokenService.class, "https://auth.docker.io");
+    tokenService =
+        buildService(DockerBearerTokenService.TokenService.class, "https://auth.docker.io");
     dockerRegistryService =
         buildService(
             DockerRegistryClient.DockerRegistryService.class, "https://registry-1.docker.io");
@@ -92,7 +89,7 @@ void getTagsWithQueryParams() {
         Retrofit2SyncCall.executeCall(
             dockerRegistryService.get(tagsPath, getBearerToken(), "Spinnaker", queryMap));
     String nextLink = response.headers().get("link");
-    assert nextLink != null;
+    assertThat(nextLink).isNotNull();
   }
 
   private String getBearerToken() {
@@ -114,14 +111,4 @@ private static <T> T buildService(Class<T> type, String baseUrl) {
         .build()
         .create(type);
   }
-
-  private interface TokenService {
-    @GET("/{path}")
-    @Headers({"Docker-Distribution-API-Version: registry/2.0"})
-    Call<DockerBearerToken> getToken(
-        @Path(value = "path", encoded = true) String path,
-        @Query(value = "service") String service,
-        @Query(value = "scope") String scope,
-        @Header("User-Agent") String agent);
-  }
 }