fix(artifacts): Find from execution should use default artifact (spin…

…naker#2905) The public method ArtifactResolver.resolveSingleArtifact should handle trying to match the artifact in the context as well as falling back to a prior execution or the default artifact instead of making callers do that. Pull the matching logic into the private function matchSingleArtifact then have resolveSingleArtifact call matchSingleArtifact and then do the defaulting if the artifact isn't resolved. We can pull this logic out of resolveExpectedArtifacts and have it just call resolveSingleArtifact.
sicp618 · May 10, 2019 · c64753f · c64753f
1 parent 4dd68fe
commit c64753f
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 28 deletions.
diff --git a/...com/netflix/spinnaker/orca/clouddriver/tasks/artifacts/FindArtifactFromExecutionTask.java b/...com/netflix/spinnaker/orca/clouddriver/tasks/artifacts/FindArtifactFromExecutionTask.java
@@ -58,7 +58,7 @@ public TaskResult execute(@Nonnull Stage stage) {
 
     List<Artifact> priorArtifacts = artifactResolver.getArtifactsForPipelineId(pipeline, executionOptions.toCriteria());
 
-    Artifact match = artifactResolver.resolveSingleArtifact(expectedArtifact, priorArtifacts, false);
+    Artifact match = artifactResolver.resolveSingleArtifact(expectedArtifact, priorArtifacts, null, false);
 
     if (match == null) {
       outputs.put("exception", "No artifact matching " + expectedArtifact + " found among " + priorArtifacts);

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactResolver.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactResolver.java
@@ -220,14 +220,30 @@ public void resolveArtifacts(@Nonnull Map<String, Object> pipeline) {
     }
   }
 
-  public Artifact resolveSingleArtifact(ExpectedArtifact expectedArtifact, List<Artifact> possibleMatches, boolean requireUniqueMatches) {
+  public Artifact resolveSingleArtifact(ExpectedArtifact expectedArtifact, List<Artifact> possibleMatches, List<Artifact> priorArtifacts, boolean requireUniqueMatches) {
+    Artifact resolved = matchSingleArtifact(expectedArtifact, possibleMatches, requireUniqueMatches);
+
+    if (resolved == null && expectedArtifact.isUsePriorArtifact() && priorArtifacts != null) {
+      resolved = matchSingleArtifact(expectedArtifact, priorArtifacts, requireUniqueMatches);
+      expectedArtifact.setBoundArtifact(resolved);
+    }
+
+    if (resolved == null && expectedArtifact.isUseDefaultArtifact() && expectedArtifact.getDefaultArtifact() != null) {
+      resolved = expectedArtifact.getDefaultArtifact();
+      expectedArtifact.setBoundArtifact(resolved);
+    }
+
+    return resolved;
+  }
+
+  private Artifact matchSingleArtifact(ExpectedArtifact expectedArtifact, List<Artifact> possibleMatches, boolean requireUniqueMatches) {
     if (expectedArtifact.getBoundArtifact() != null) {
       return expectedArtifact.getBoundArtifact();
     }
     List<Artifact> matches = possibleMatches
-        .stream()
-        .filter(expectedArtifact::matches)
-        .collect(toList());
+      .stream()
+      .filter(expectedArtifact::matches)
+      .collect(toList());
     Artifact result;
     switch (matches.size()) {
       case 0:
@@ -252,29 +268,9 @@ public Set<Artifact> resolveExpectedArtifacts(List<ExpectedArtifact> expectedArt
 
   public LinkedHashSet<Artifact> resolveExpectedArtifacts(List<ExpectedArtifact> expectedArtifacts, List<Artifact> receivedArtifacts, List<Artifact> priorArtifacts, boolean requireUniqueMatches) {
     LinkedHashSet<Artifact> resolvedArtifacts = new LinkedHashSet<>();
-    LinkedHashSet<ExpectedArtifact> unresolvedExpectedArtifacts = new LinkedHashSet<>();
 
     for (ExpectedArtifact expectedArtifact : expectedArtifacts) {
-      Artifact resolved = resolveSingleArtifact(expectedArtifact, receivedArtifacts, requireUniqueMatches);
-      if (resolved != null) {
-        resolvedArtifacts.add(resolved);
-      } else {
-        unresolvedExpectedArtifacts.add(expectedArtifact);
-      }
-    }
-
-    for (ExpectedArtifact expectedArtifact : unresolvedExpectedArtifacts) {
-      Artifact resolved = null;
-      if (expectedArtifact.isUsePriorArtifact() && priorArtifacts != null) {
-        resolved = resolveSingleArtifact(expectedArtifact, priorArtifacts, requireUniqueMatches);
-        expectedArtifact.setBoundArtifact(resolved);
-      }
-
-      if (resolved == null && expectedArtifact.isUseDefaultArtifact() && expectedArtifact.getDefaultArtifact() != null) {
-        resolved = expectedArtifact.getDefaultArtifact();
-        expectedArtifact.setBoundArtifact(resolved);
-      }
-
+      Artifact resolved = resolveSingleArtifact(expectedArtifact, receivedArtifacts, priorArtifacts, requireUniqueMatches);
       if (resolved == null) {
         throw new InvalidRequestException(format("Unmatched expected artifact %s could not be resolved.", expectedArtifact));
       } else {

diff --git a/...core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactResolverSpec.groovy b/...core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactResolverSpec.groovy
@@ -242,7 +242,7 @@ class ArtifactResolverSpec extends Specification {
   def "should find a matching artifact for #expected"() {
     when:
     def artifactResolver = makeArtifactResolver()
-    def artifact = artifactResolver.resolveSingleArtifact(expected, existing, true)
+    def artifact = artifactResolver.resolveSingleArtifact(expected, existing, null, true)
 
     then:
     artifact == desired
@@ -258,7 +258,7 @@ class ArtifactResolverSpec extends Specification {
   def "should fail find a matching artifact for #expected"() {
     when:
     def artifactResolver = makeArtifactResolver()
-    def artifact = artifactResolver.resolveSingleArtifact(expected, existing, true)
+    def artifact = artifactResolver.resolveSingleArtifact(expected, existing, null, true)
 
     then:
     artifact == null