From 98b814b5642476ed9411d533f36ef4aa890c16f3 Mon Sep 17 00:00:00 2001 From: xibz Date: Tue, 19 Sep 2023 15:19:43 -0500 Subject: [PATCH] fix(artifacts): Resolving is broken in multiple places (#4526) * fix(artifacts): Resolving is broken in multiple places 8df68b79cf1 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 * fixup! fix(artifacts): Resolving is broken in multiple places Signed-off-by: benjamin-j-powell * fixup! fix(artifacts): Resolving is broken in multiple places Signed-off-by: benjamin-j-powell --------- Signed-off-by: benjamin-j-powell Co-authored-by: Benevolent Benjamin Powell Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- .../orca/pipeline/util/ArtifactUtils.java | 35 ++++++++- .../pipeline/util/ArtifactUtilsSpec.groovy | 78 +++++++++++++++++++ 2 files changed, 111 insertions(+), 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 index f57085a5d2..1fb5b059ea 100644 --- 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 getArtifactsForPipelineIdWithoutStageRef( .orElse(Collections.emptyList()); } + private List getExpectedArtifactIdsFromMap(Map trigger) { + List expectedArtifactIds = (List) trigger.get("expectedArtifactIds"); + return (expectedArtifactIds != null) ? expectedArtifactIds : emptyList(); + } + public void resolveArtifacts(Map pipeline) { Map trigger = (Map) pipeline.get("trigger"); - List expectedArtifactIds = - (List) trigger.getOrDefault("expectedArtifactIds", emptyList()); + List triggers = Optional.ofNullable((List) pipeline.get("triggers")).orElse(emptyList()); + Set 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) 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 expectedArtifactIds = new ArrayList<>(expectedArtifactIdsListConcat); ImmutableList 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 index 97ff6240d8..e8fc88b65b 100644 --- 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 extractTriggerArtifacts(Map trigger) { return objectMapper.convertValue(trigger.artifacts, new TypeReference>(){}); }