From b66b3e090b13e8d7ba0b5eefe53412486d49a6a3 Mon Sep 17 00:00:00 2001 From: Nemesis Osorio Date: Wed, 25 Oct 2023 16:58:19 -0600 Subject: [PATCH] refactor(artifacts): resolve artifacts methods --- .../orca/pipeline/util/ArtifactUtils.java | 146 +++++------------- .../pipeline/util/ArtifactUtilsSpec.groovy | 78 ---------- 2 files changed, 41 insertions(+), 183 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 abaa0a27ee..a0026cb1cf 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,16 +36,13 @@ 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; @@ -215,20 +212,7 @@ public List getArtifactsForPipelineIdWithoutStageRef( */ public void validateArtifactConstraintsFromTrigger(Map pipeline) { Map trigger = (Map) pipeline.get("trigger"); - List expectedArtifactIds = - (List) trigger.getOrDefault("expectedArtifactIds", emptyList()); - - ImmutableList expectedArtifacts = - Optional.ofNullable((List) pipeline.get("expectedArtifacts")) - .map(Collection::stream) - .orElse(Stream.empty()) - .map(it -> objectMapper.convertValue(it, ExpectedArtifact.class)) - .filter( - artifact -> - expectedArtifactIds.contains(artifact.getId()) - || artifact.isUseDefaultArtifact() - || artifact.isUsePriorArtifact()) - .collect(toImmutableList()); + ImmutableList expectedArtifacts = filterExpectedArtifacts(pipeline, trigger); ImmutableSet receivedArtifacts = concatReceivedArtifacts(pipeline, trigger); @@ -237,19 +221,6 @@ public void validateArtifactConstraintsFromTrigger(Map pipeline) .resolveExpectedArtifacts(expectedArtifacts); } - private ImmutableSet concatReceivedArtifacts( - Map pipeline, Map trigger) { - return Stream.concat( - Optional.ofNullable((List) pipeline.get("receivedArtifacts")) - .map(Collection::stream) - .orElse(Stream.empty()), - Optional.ofNullable((List) trigger.get("artifacts")) - .map(Collection::stream) - .orElse(Stream.empty())) - .map(it -> objectMapper.convertValue(it, Artifact.class)) - .collect(toImmutableSet()); - } - /** * This will only resolve the expected artifacts that match received artifacts.
* If the expected artifact is not present in received artifacts, it is not resolved.
@@ -280,86 +251,14 @@ public void resolveArtifactsFromTrigger(Map pipeline) { /* requireUniqueMatches= */ true) .resolveExpectedArtifacts(expectedArtifacts, false); - ImmutableSet allArtifacts = - ImmutableSet.builder() - .addAll(receivedArtifacts) - .addAll(resolveResult.getResolvedArtifacts()) - .build(); - - try { - trigger.put( - "artifacts", - objectMapper.readValue(objectMapper.writeValueAsString(allArtifacts), List.class)); - trigger.put( - "expectedArtifacts", - objectMapper.readValue( - objectMapper.writeValueAsString(resolveResult.getResolvedExpectedArtifacts()), - List.class)); - trigger.put( - "resolvedExpectedArtifacts", - objectMapper.readValue( - objectMapper.writeValueAsString(resolveResult.getResolvedExpectedArtifacts()), - List.class)); // Add the actual expectedArtifacts we included in the ids. - } catch (IOException e) { - throw new ArtifactResolutionException( - "Failed to store artifacts in trigger: " + e.getMessage(), e); - } - } - - private List getExpectedArtifactIdsFromMap(Map trigger) { - List expectedArtifactIds = (List) trigger.get("expectedArtifactIds"); - return (expectedArtifactIds != null) ? expectedArtifactIds : emptyList(); + saveArtifactsInTrigger(trigger, receivedArtifacts, resolveResult); } public void resolveArtifacts(Map pipeline) { Map trigger = (Map) pipeline.get("trigger"); - 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) - .orElse(Stream.empty()) - .map(it -> objectMapper.convertValue(it, ExpectedArtifact.class)) - .filter( - artifact -> - expectedArtifactIds.contains(artifact.getId()) - || artifact.isUseDefaultArtifact() - || artifact.isUsePriorArtifact()) - .collect(toImmutableList()); + ImmutableList expectedArtifacts = filterExpectedArtifacts(pipeline, trigger); - ImmutableSet receivedArtifacts = - Stream.concat( - Optional.ofNullable((List) pipeline.get("receivedArtifacts")) - .map(Collection::stream) - .orElse(Stream.empty()), - Optional.ofNullable((List) trigger.get("artifacts")) - .map(Collection::stream) - .orElse(Stream.empty())) - .map(it -> objectMapper.convertValue(it, Artifact.class)) - .collect(toImmutableSet()); + ImmutableSet receivedArtifacts = concatReceivedArtifacts(pipeline, trigger); ArtifactResolver.ResolveResult resolveResult = ArtifactResolver.getInstance( @@ -368,6 +267,43 @@ public void resolveArtifacts(Map pipeline) { /* requireUniqueMatches= */ true) .resolveExpectedArtifacts(expectedArtifacts); + saveArtifactsInTrigger(trigger, receivedArtifacts, resolveResult); + } + + private ImmutableList filterExpectedArtifacts( + Map pipeline, Map trigger) { + List expectedArtifactIds = + (List) trigger.getOrDefault("expectedArtifactIds", emptyList()); + + return Optional.ofNullable((List) pipeline.get("expectedArtifacts")) + .map(Collection::stream) + .orElse(Stream.empty()) + .map(it -> objectMapper.convertValue(it, ExpectedArtifact.class)) + .filter( + artifact -> + expectedArtifactIds.contains(artifact.getId()) + || artifact.isUseDefaultArtifact() + || artifact.isUsePriorArtifact()) + .collect(toImmutableList()); + } + + private ImmutableSet concatReceivedArtifacts( + Map pipeline, Map trigger) { + return Stream.concat( + Optional.ofNullable((List) pipeline.get("receivedArtifacts")) + .map(Collection::stream) + .orElse(Stream.empty()), + Optional.ofNullable((List) trigger.get("artifacts")) + .map(Collection::stream) + .orElse(Stream.empty())) + .map(it -> objectMapper.convertValue(it, Artifact.class)) + .collect(toImmutableSet()); + } + + private void saveArtifactsInTrigger( + Map trigger, + ImmutableSet receivedArtifacts, + ArtifactResolver.ResolveResult resolveResult) { ImmutableSet allArtifacts = ImmutableSet.builder() .addAll(receivedArtifacts) 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 ef7f5a80a5..fedf066359 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,7 +19,6 @@ 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 @@ -493,83 +492,6 @@ 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>(){}); }