refactor(kubernetes): Clean up KubernetesArtifactConverter (#4502)

* refactor(kubernetes): Make KubernetesCoordinates immutable Luckily there are no existing uses that mutate it, so we can greatly reduce its API by making it a Value class. * refactor(kubernetes): Clean up KubernetesArtifactConverter There are a bunch of functions there that are no longer used, so we can significantly clean up that class (and implementations). Also move getAccount to the only place where it is used. * refactor(kubernetes): Make implementations of converter singletons It's confusing how we store an instance of each artifact converter on each KubernetesResourceProperties instance. The properties should only care whether the resource is versioned; the deployer logic can figure out what artifact converter to use based on whether the resource is versioned (and whether it has been overriden by an annotation). The goal here is less to reduce the number of created objects (which is a side benefit, though probably minimal), but more to keep KubernetesResourceProperties more focused, with the goal of one day maybe merging it with KubernetesKindProperties so we don't have so many of these classes describing kinds floating around.
spinnaker · Apr 10, 2020 · fc235c3 · fc235c3
1 parent 5913508
commit fc235c3
Show file tree

Hide file tree

Showing 9 changed files with 40 additions and 77 deletions.
diff --git a/.../main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacer.java b/.../main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacer.java
@@ -35,6 +35,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import javax.annotation.Nonnull;
 import javax.annotation.ParametersAreNonnullByDefault;
 import lombok.Value;
@@ -83,7 +84,7 @@ private static ImmutableList<Artifact> filterKubernetesArtifactsByNamespaceAndAc
               }
 
               boolean accountMatches;
-              String artifactAccount = KubernetesArtifactConverter.getAccount(a);
+              String artifactAccount = getAccount(a);
               // If the artifact fails to provide an account, we'll assume this was unintentional
               // and match anyways
               accountMatches =
@@ -94,6 +95,15 @@ private static ImmutableList<Artifact> filterKubernetesArtifactsByNamespaceAndAc
         .collect(toImmutableList());
   }
 
+  private static String getAccount(Artifact artifact) {
+    String account = "";
+    Map<String, Object> metadata = artifact.getMetadata();
+    if (metadata != null) {
+      account = (String) metadata.getOrDefault("account", "");
+    }
+    return account;
+  }
+
   @Nonnull
   public ReplaceResult replaceAll(
       KubernetesManifest input, List<Artifact> artifacts, String namespace, String account) {

diff --git a/...com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/KubernetesArtifactConverter.java b/...com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/KubernetesArtifactConverter.java
@@ -18,49 +18,26 @@
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.artifact;
 
 import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesCloudProvider;
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesCoordinates;
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest;
 import com.netflix.spinnaker.clouddriver.model.ArtifactProvider;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
-import java.util.Map;
 
 public abstract class KubernetesArtifactConverter {
+  // Prevent subclassing from outside the package
+  KubernetesArtifactConverter() {}
+
+  public static KubernetesArtifactConverter getInstance(boolean versioned) {
+    return versioned
+        ? KubernetesVersionedArtifactConverter.INSTANCE
+        : KubernetesUnversionedArtifactConverter.INSTANCE;
+  }
+
   public abstract Artifact toArtifact(
       ArtifactProvider artifactProvider, KubernetesManifest manifest, String account);
 
-  public abstract KubernetesCoordinates toCoordinates(Artifact artifact);
-
   public abstract String getDeployedName(Artifact artifact);
 
-  protected String getType(KubernetesManifest manifest) {
+  protected final String getType(KubernetesManifest manifest) {
     return String.join("/", KubernetesCloudProvider.ID, manifest.getKind().toString());
   }
-
-  protected KubernetesKind getKind(Artifact artifact) {
-    String[] split = artifact.getType().split("/", -1);
-    if (split.length != 2) {
-      throw new IllegalArgumentException("Not a kubernetes artifact: " + artifact);
-    }
-
-    if (!split[0].equals(KubernetesCloudProvider.ID)) {
-      throw new IllegalArgumentException("Not a kubernetes artifact: " + artifact);
-    }
-
-    return KubernetesKind.fromString(split[1]);
-  }
-
-  protected String getNamespace(Artifact artifact) {
-    return artifact.getLocation();
-  }
-
-  public static String getAccount(Artifact artifact) {
-    String account = "";
-    Map<String, Object> metadata = artifact.getMetadata();
-    if (metadata != null) {
-      account = (String) metadata.getOrDefault("account", "");
-    }
-
-    return account;
-  }
 }
diff --git a/.../spinnaker/clouddriver/kubernetes/v2/artifact/KubernetesUnversionedArtifactConverter.java b/.../spinnaker/clouddriver/kubernetes/v2/artifact/KubernetesUnversionedArtifactConverter.java
@@ -17,14 +17,18 @@
 
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.artifact;
 
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesCoordinates;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest;
 import com.netflix.spinnaker.clouddriver.model.ArtifactProvider;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import java.util.HashMap;
 import java.util.Map;
 
-public class KubernetesUnversionedArtifactConverter extends KubernetesArtifactConverter {
+final class KubernetesUnversionedArtifactConverter extends KubernetesArtifactConverter {
+  static final KubernetesUnversionedArtifactConverter INSTANCE =
+      new KubernetesUnversionedArtifactConverter();
+
+  private KubernetesUnversionedArtifactConverter() {}
+
   @Override
   public Artifact toArtifact(
       ArtifactProvider provider, KubernetesManifest manifest, String account) {
@@ -42,15 +46,6 @@ public Artifact toArtifact(
         .build();
   }
 
-  @Override
-  public KubernetesCoordinates toCoordinates(Artifact artifact) {
-    return KubernetesCoordinates.builder()
-        .kind(getKind(artifact))
-        .namespace(getNamespace(artifact))
-        .name(artifact.getName())
-        .build();
-  }
-
   @Override
   public String getDeployedName(Artifact artifact) {
     return artifact.getName();

diff --git a/...ix/spinnaker/clouddriver/kubernetes/v2/artifact/KubernetesVersionedArtifactConverter.java b/...ix/spinnaker/clouddriver/kubernetes/v2/artifact/KubernetesVersionedArtifactConverter.java
@@ -18,7 +18,6 @@
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.artifact;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesCoordinates;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest;
 import com.netflix.spinnaker.clouddriver.model.ArtifactProvider;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
@@ -31,8 +30,13 @@
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
-public class KubernetesVersionedArtifactConverter extends KubernetesArtifactConverter {
-  private static final ObjectMapper objectMapper = new ObjectMapper();
+final class KubernetesVersionedArtifactConverter extends KubernetesArtifactConverter {
+  static final KubernetesVersionedArtifactConverter INSTANCE =
+      new KubernetesVersionedArtifactConverter();
+
+  private final ObjectMapper objectMapper = new ObjectMapper();
+
+  private KubernetesVersionedArtifactConverter() {}
 
   @Override
   public Artifact toArtifact(
@@ -53,15 +57,6 @@ public Artifact toArtifact(
         .build();
   }
 
-  @Override
-  public KubernetesCoordinates toCoordinates(Artifact artifact) {
-    return KubernetesCoordinates.builder()
-        .kind(getKind(artifact))
-        .name(getDeployedName(artifact))
-        .namespace(getNamespace(artifact))
-        .build();
-  }
-
   @Override
   public String getDeployedName(Artifact artifact) {
     return getDeployedName(artifact.getName(), artifact.getVersion());

diff --git a/...va/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesCoordinates.java b/...va/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesCoordinates.java
@@ -18,17 +18,13 @@
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description;
 
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
-import lombok.AllArgsConstructor;
 import lombok.Builder;
-import lombok.Data;
-import lombok.NoArgsConstructor;
+import lombok.Value;
 
 @Builder
-@AllArgsConstructor
-@NoArgsConstructor
-@Data
+@Value
 public class KubernetesCoordinates {
-  KubernetesKind kind;
-  String namespace;
-  String name;
+  private final KubernetesKind kind;
+  private final String namespace;
+  private final String name;
 }
diff --git a/...netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesResourceProperties.java b/...netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesResourceProperties.java
@@ -21,8 +21,6 @@
 
 import com.netflix.spinnaker.clouddriver.kubernetes.config.CustomKubernetesResource;
 import com.netflix.spinnaker.clouddriver.kubernetes.description.SpinnakerKind;
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.artifact.KubernetesUnversionedArtifactConverter;
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.artifact.KubernetesVersionedArtifactConverter;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.handler.CustomKubernetesHandlerFactory;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.handler.KubernetesHandler;
@@ -35,10 +33,6 @@
 public class KubernetesResourceProperties {
   private final KubernetesHandler handler;
   private final boolean versioned;
-  private final KubernetesVersionedArtifactConverter versionedConverter =
-      new KubernetesVersionedArtifactConverter();
-  private final KubernetesUnversionedArtifactConverter unversionedConverter =
-      new KubernetesUnversionedArtifactConverter();
 
   public KubernetesResourceProperties(KubernetesHandler handler, boolean versioned) {
     this.handler = handler;

diff --git a/...ix/spinnaker/clouddriver/kubernetes/v2/op/manifest/KubernetesDeployManifestOperation.java b/...ix/spinnaker/clouddriver/kubernetes/v2/op/manifest/KubernetesDeployManifestOperation.java
@@ -159,9 +159,7 @@ public OperationResult operate(List _unused) {
       KubernetesManifestStrategy strategy = KubernetesManifestAnnotater.getStrategy(manifest);
 
       KubernetesArtifactConverter converter =
-          isVersioned(properties, strategy)
-              ? properties.getVersionedConverter()
-              : properties.getUnversionedConverter();
+          KubernetesArtifactConverter.getInstance(isVersioned(properties, strategy));
       KubernetesHandler deployer = properties.getHandler();
 
       Moniker moniker = cloneMoniker(description.getMoniker());

diff --git a/...aker/clouddriver/kubernetes/v2/artifact/KubernetesUnversionedArtifactConverterSpec.groovy b/...aker/clouddriver/kubernetes/v2/artifact/KubernetesUnversionedArtifactConverterSpec.groovy
@@ -35,7 +35,6 @@ class KubernetesUnversionedArtifactConverterSpec extends Specification {
       .build()
 
     def converter = new KubernetesUnversionedArtifactConverter()
-    converter.getKind(artifact) == kind
     converter.getDeployedName(artifact) == "$name"
 
     where:

diff --git a/...nnaker/clouddriver/kubernetes/v2/artifact/KubernetesVersionedArtifactConverterSpec.groovy b/...nnaker/clouddriver/kubernetes/v2/artifact/KubernetesVersionedArtifactConverterSpec.groovy
@@ -39,7 +39,6 @@ class KubernetesVersionedArtifactConverterSpec extends Specification {
       .build()
 
     def converter = new KubernetesVersionedArtifactConverter()
-    converter.getKind(artifact) == kind
     converter.getDeployedName(artifact) == "$name-$version"
 
     where: