fix(artifacts): Resolving is broken in multiple places (spinnaker#4526)

* fix(artifacts): Resolving is broken in multiple places 8df68b7 broke custom triggers to Spinnaker integration. Prior to this commit if `getBoundArtifactForStage` failed to find a bound artifact, resolution would happen much later by utilizing the `triggers` field in stages. However, `getBoundArtifactForStage` now throws an exception, which causes custom triggers to fail prematurely. Also if a custom trigger calls orchestrate directly and any pipeline contains `getBoundArtifactForStage`, this will throw an exception and prevent any resolution. Signed-off-by: benjamin-j-powell <[email protected]> * fixup! fix(artifacts): Resolving is broken in multiple places Signed-off-by: benjamin-j-powell <[email protected]> * fixup! fix(artifacts): Resolving is broken in multiple places Signed-off-by: benjamin-j-powell <[email protected]> --------- Signed-off-by: benjamin-j-powell <[email protected]> Co-authored-by: Benevolent Benjamin Powell <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
coveord · Sep 19, 2023 · 98b814b · 98b814b
1 parent 6ae6207
commit 98b814b
Show file tree

Hide file tree

Showing 2 changed files with 111 additions and 2 deletions.
diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java
@@ -36,13 +36,16 @@
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionCriteria;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -227,10 +230,38 @@ public List<Artifact> getArtifactsForPipelineIdWithoutStageRef(
         .orElse(Collections.emptyList());
   }
 
+  private List<String> getExpectedArtifactIdsFromMap(Map<String, Object> trigger) {
+    List<String> expectedArtifactIds = (List<String>) trigger.get("expectedArtifactIds");
+    return (expectedArtifactIds != null) ? expectedArtifactIds : emptyList();
+  }
+
   public void resolveArtifacts(Map pipeline) {
     Map<String, Object> trigger = (Map<String, Object>) pipeline.get("trigger");
-    List<?> expectedArtifactIds =
-        (List<?>) trigger.getOrDefault("expectedArtifactIds", emptyList());
+    List<?> triggers = Optional.ofNullable((List<?>) pipeline.get("triggers")).orElse(emptyList());
+    Set<String> expectedArtifactIdsListConcat =
+        new HashSet<>(getExpectedArtifactIdsFromMap(trigger));
+
+    // Due to 8df68b79cf1 getBoundArtifactForStage now does resolution which
+    // can potentially return null artifact back, which will throw an exception
+    // for stages that expect a non-null value. Before this commit, when
+    // getBoundArtifactForStage was called, the method would just retrieve the
+    // bound artifact from the stage context, and return the appropriate
+    // artifact back. This change prevents tasks like CreateBakeManifestTask
+    // from working properly, if null is returned.
+    //
+    // reference: https://github.com/spinnaker/orca/pull/4397
+    triggers.stream()
+        .map(it -> (Map<String, Object>) it)
+        // This filter prevents multiple triggers from adding its
+        // expectedArtifactIds unless it is the expected trigger type that was
+        // triggered
+        //
+        // reference: https://github.com/spinnaker/orca/pull/4322
+        .filter(it -> trigger.getOrDefault("type", "").equals(it.get("type")))
+        .map(this::getExpectedArtifactIdsFromMap)
+        .forEach(expectedArtifactIdsListConcat::addAll);
+
+    final List<String> expectedArtifactIds = new ArrayList<>(expectedArtifactIdsListConcat);
     ImmutableList<ExpectedArtifact> expectedArtifacts =
         Optional.ofNullable((List<?>) pipeline.get("expectedArtifacts"))
             .map(Collection::stream)

diff --git a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy
@@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.pipeline.util
 
 import com.fasterxml.jackson.core.type.TypeReference
 import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.kork.artifacts.ArtifactTypes
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
 import com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
@@ -515,6 +516,83 @@ class ArtifactUtilsSpec extends Specification {
     initialArtifacts == finalArtifacts
   }
 
+  def "should find artifact if triggers is present in pipeline"() {
+    given:
+    def defaultArtifact = Artifact.builder()
+        .customKind(true)
+        .build()
+
+    def matchArtifact = Artifact.builder()
+        .name("my-pipeline-artifact")
+        .type("embedded/base64")
+        .reference("aGVsbG8gd29ybGQK")
+        .build()
+
+    def expectedArtifact = ExpectedArtifact.builder()
+        .usePriorArtifact(false)
+        .useDefaultArtifact(false)
+        .id("my-id")
+        .defaultArtifact(defaultArtifact)
+        .matchArtifact(matchArtifact)
+        .build()
+
+    def expectedArtifact2 = ExpectedArtifact.builder()
+        .usePriorArtifact(false)
+        .useDefaultArtifact(false)
+        .id("my-id-2")
+        .defaultArtifact(defaultArtifact)
+        .matchArtifact(matchArtifact)
+        .build()
+
+    def pipeline = [
+        "id": "abc",
+        "stages": [
+            stage {
+              expectedArtifacts: [expectedArtifact]
+              inputArtifacts: [
+                  "id": "my-id"
+              ]
+            }
+        ],
+        expectedArtifacts: [
+          expectedArtifact
+        ],
+        trigger: [
+            artifacts: [
+               Artifact.builder()
+                .type(ArtifactTypes.EMBEDDED_BASE64.getMimeType())
+                .name(matchArtifact.getName())
+                .reference(matchArtifact.getReference())
+                .build()
+            ],
+            type: "some-type"
+        ],
+        triggers: [
+            [
+                enabled: true,
+                expectedArtifactIds: [
+                    expectedArtifact.getId()
+                ],
+                type: "some-type"
+            ],
+            [
+                enabled: true,
+                expectedArtifactIds: [
+                    expectedArtifact2.getId()
+                ],
+                type: "some-other-type"
+            ]
+        ]
+    ]
+
+    def pipelineMap = getObjectMapper().convertValue(pipeline, Map.class)
+    when:
+     makeArtifactUtils().resolveArtifacts(pipelineMap)
+
+    then:
+    pipelineMap.trigger.resolvedExpectedArtifacts.size() == 1
+  }
+
   private List<Artifact> extractTriggerArtifacts(Map<String, Object> trigger) {
     return objectMapper.convertValue(trigger.artifacts, new TypeReference<List<Artifact>>(){});
   }