fix(artifacts): Parent and child pipeline artifact resolution #4575

xibz · 2023-10-26T22:42:34Z

This fix allows us to revert #4397
since that makes retrieving bound artifacts actually more strict for
most use cases. So this commit addresses the root cause that that PR was
trying to implement.

When a parent pipeline triggers a child pipeline, the child pipeline should be able to
resolve incoming artifacts properly. Right now that does not happen
because expectedArtifactIds is needed in the trigger section,
otherwise no resolution will occur.

This change fixes the parent payload to include expectedArtifactIds by
viewing the child pipeline's top level expected artifacts and finding
the intersection between expected artifacts and artifacts to be sent.
With that intersection we can grab the expected artifact ids and
properly set the the necessary field to allow for child pipelines to
ingest the artifact.

@nemesisOsorio

Also, we can probably still utilize some of #4570 and still revert the changes that were introduced due to not properly handling the root cause

sergio-quintero · 2023-10-26T23:40:15Z

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy

+    expectedArtifactIds.addAll(pipelineConfig.get("expectedArtifacts", []).collectMany {
+      def expectedArtifact = objectMapper.convertValue(it, ExpectedArtifact)
+      return trigger.artifacts.findAll {
+        // The idea here is we are only interested in matching against incoming
+        // artifacts. So if the expected artifact does not match on any incoming
+        // artifact, we will simply not add the expected artifact ID to the
+        // array. This is very similar to how echo works.
+        def artifact = objectMapper.convertValue(it, Artifact)
+        return expectedArtifact.matches(artifact)
+      }.collect {
+        return expectedArtifact.id
+      }
+    } as String[])
+    trigger.expectedArtifactIds = expectedArtifactIds


Would it make sense to wrap this code under the condition when the trigger is from a pipeline stage? Maybe add it under line 130 where it checks if (parentPipelineStageId != null) ?

After reviewing this, we shouldn't move this code because if an expected artifact is added to the pipeline json without an artifact constraint in a trigger, it will not be able to resolve it.

sergio-quintero

LGTM

nemesisOsorio

can we please backport this all the way down to 1.30?

nemesisOsorio · 2023-10-27T00:07:25Z

@jervi can you please take a look?

dbyron-sf · 2023-10-30T20:23:44Z

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy

@@ -133,6 +136,25 @@ class DependentPipelineStarter implements ApplicationContextAware {
    // This is required for template source with jinja expressions
    trigger.artifacts = pipelineConfig.receivedArtifacts

+    // expectedArtifacts for triggers are defined at the top level of the


Trying to follow this, I started above with:

def expectedArtifactIds = pipelineConfig.get("triggers", []).findAll { it.type == "pipeline" && it.pipeline == parentPipeline.pipelineConfigId } collectMany { it.expectedArtifactIds ?: [] }

I'd love some help connecting what that block does to here.

This fix allows us to revert spinnaker#4397 since that makes retrieving bound artifacts actually more strict for most use cases. So this commit addresses the root cause that that PR was trying to implement. When a parent pipeline triggers a child pipeline, the child pipeline should be able to resolve incoming artifacts properly. Right now that does not happen because `expectedArtifactIds` is needed in the trigger section, otherwise no resolution will occur. This change fixes the parent payload to include `expectedArtifactIds` by viewing the child pipeline's top level expected artifacts and finding the intersection between expected artifacts and artifacts to be sent. With that intersection we can grab the expected artifact ids and properly set the the necessary field to allow for child pipelines to ingest the artifact. Signed-off-by: benjamin-j-powell <bjp@apple.com>

jervi · 2023-10-31T08:49:21Z

Sorry, I'm late to the game! I think this looks good as part of a much overdue refresh of the artifact handling in Spinnaker. Thank you for spending time on this, @xibz and @nemesisOsorio 🚀

link108 · 2023-10-31T17:12:07Z

@Mergifyio backport release-1.30.x release-1.31.x release-1.32.x

mergify · 2023-10-31T17:12:21Z

backport release-1.30.x release-1.31.x release-1.32.x

✅ Backports have been created

#4581 fix(artifacts): Parent and child pipeline artifact resolution (backport #4575) has been created for branch release-1.30.x
#4582 fix(artifacts): Parent and child pipeline artifact resolution (backport #4575) has been created for branch release-1.31.x
#4583 fix(artifacts): Parent and child pipeline artifact resolution (backport #4575) has been created for branch release-1.32.x

This fix allows us to revert #4397 since that makes retrieving bound artifacts actually more strict for most use cases. So this commit addresses the root cause that that PR was trying to implement. When a parent pipeline triggers a child pipeline, the child pipeline should be able to resolve incoming artifacts properly. Right now that does not happen because `expectedArtifactIds` is needed in the trigger section, otherwise no resolution will occur. This change fixes the parent payload to include `expectedArtifactIds` by viewing the child pipeline's top level expected artifacts and finding the intersection between expected artifacts and artifacts to be sent. With that intersection we can grab the expected artifact ids and properly set the the necessary field to allow for child pipelines to ingest the artifact. Signed-off-by: benjamin-j-powell <bjp@apple.com> Co-authored-by: Benevolent Benjamin Powell <benjamin_j_powell@apple.com> (cherry picked from commit 2e3c5f0)

…ker#4575) This fix allows us to revert spinnaker#4397 since that makes retrieving bound artifacts actually more strict for most use cases. So this commit addresses the root cause that that PR was trying to implement. When a parent pipeline triggers a child pipeline, the child pipeline should be able to resolve incoming artifacts properly. Right now that does not happen because `expectedArtifactIds` is needed in the trigger section, otherwise no resolution will occur. This change fixes the parent payload to include `expectedArtifactIds` by viewing the child pipeline's top level expected artifacts and finding the intersection between expected artifacts and artifacts to be sent. With that intersection we can grab the expected artifact ids and properly set the the necessary field to allow for child pipelines to ingest the artifact. Signed-off-by: benjamin-j-powell <bjp@apple.com> Co-authored-by: Benevolent Benjamin Powell <benjamin_j_powell@apple.com>

#4575) (#4583) Co-authored-by: Benevolent Benjamin Powell <benjamin_j_powell@apple.com> Co-authored-by: xibz <impactbchang@gmail.com> fix allows us to revert #4397

#4575) (#4582) Co-authored-by: Benevolent Benjamin Powell <benjamin_j_powell@apple.com> Co-authored-by: xibz <impactbchang@gmail.com> fix allows us to revert #4397

#4575) (#4581) Co-authored-by: Benevolent Benjamin Powell <benjamin_j_powell@apple.com> Co-authored-by: xibz <impactbchang@gmail.com> fix allows us to revert #4397

spinnaker#4575) (spinnaker#4581) Co-authored-by: Benevolent Benjamin Powell <benjamin_j_powell@apple.com> Co-authored-by: xibz <impactbchang@gmail.com> fix allows us to revert spinnaker#4397

sergio-quintero reviewed Oct 26, 2023

View reviewed changes

sergio-quintero approved these changes Oct 27, 2023

View reviewed changes

nemesisOsorio approved these changes Oct 27, 2023

View reviewed changes

dbyron-sf reviewed Oct 30, 2023

View reviewed changes

xibz force-pushed the more-artifact-resolution branch 2 times, most recently from e2bc704 to 33864c5 Compare October 30, 2023 20:31

xibz changed the title ~~fix(artifacts): Parent and child pipeline artifact resolution (#1495)~~ fix(artifacts): Parent and child pipeline artifact resolution Oct 30, 2023

xibz force-pushed the more-artifact-resolution branch from 33864c5 to 710bf4c Compare October 30, 2023 20:54

dbyron-sf approved these changes Oct 30, 2023

View reviewed changes

dbyron-sf added the ready to merge Approved and ready for merge label Oct 30, 2023

mergify bot added the auto merged Merged automatically by a bot label Oct 30, 2023

mergify bot merged commit 2e3c5f0 into spinnaker:master Oct 30, 2023
4 checks passed

spinnakerbot added the target-release/1.33 label Oct 30, 2023

nemesisOsorio mentioned this pull request Oct 30, 2023

fix(artifacts): Automated triggers with artifact constraints are broken if you have 2 or more of the same type #4579

Merged

link108 added the backport-candidate Add to PRs to designate release branch patch candidates. label Oct 31, 2023

mergify bot mentioned this pull request Oct 31, 2023

fix(artifacts): Parent and child pipeline artifact resolution (backport #4575) #4581

Merged

mergify bot mentioned this pull request Oct 31, 2023

fix(artifacts): Parent and child pipeline artifact resolution (backport #4575) #4582

Merged

mergify bot mentioned this pull request Oct 31, 2023

fix(artifacts): Parent and child pipeline artifact resolution (backport #4575) #4583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(artifacts): Parent and child pipeline artifact resolution #4575

fix(artifacts): Parent and child pipeline artifact resolution #4575

xibz commented Oct 26, 2023 •

edited by dbyron-sf

Loading

sergio-quintero Oct 26, 2023

sergio-quintero Oct 27, 2023

sergio-quintero left a comment

nemesisOsorio left a comment

nemesisOsorio commented Oct 27, 2023

dbyron-sf Oct 30, 2023

jervi commented Oct 31, 2023

link108 commented Oct 31, 2023

mergify bot commented Oct 31, 2023 •

edited

Loading

fix(artifacts): Parent and child pipeline artifact resolution #4575

fix(artifacts): Parent and child pipeline artifact resolution #4575

Conversation

xibz commented Oct 26, 2023 • edited by dbyron-sf Loading

sergio-quintero Oct 26, 2023

Choose a reason for hiding this comment

sergio-quintero Oct 27, 2023

Choose a reason for hiding this comment

sergio-quintero left a comment

Choose a reason for hiding this comment

nemesisOsorio left a comment

Choose a reason for hiding this comment

nemesisOsorio commented Oct 27, 2023

dbyron-sf Oct 30, 2023

Choose a reason for hiding this comment

jervi commented Oct 31, 2023

link108 commented Oct 31, 2023

mergify bot commented Oct 31, 2023 • edited Loading

✅ Backports have been created

xibz commented Oct 26, 2023 •

edited by dbyron-sf

Loading

mergify bot commented Oct 31, 2023 •

edited

Loading