fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type #4489

nemesisOsorio · 2023-07-19T23:27:03Z

Expected artifacts can be used in automated triggers and stages,
as in the following example:

if we trigger the previous pipeline with a pipeline stage like the next image

you will get the following error even if the parent pipeline provides the artifact

in the example, I'm using a Find Artifacts From Resource (Manifest) stage
and producing a docker image as an artifact, I believe this is a similar case for building
the docker image using Jenkins like is mentioned in spinnaker/spinnaker#6850

Before the following PRs (i.e. in spinnaker < 1.30) Deploy (Manifest) stage was working
#4322
#4397
#4404

the intent of this PR is to restore the previous behavior

Here are the pipelines

pr-pipelines.zip

dbyron-sf · 2023-07-20T15:33:41Z

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

@@ -93,7 +98,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
      parameters           : [:],
      strategy             : suppliedParameters.strategy == true,
      correlationId        : "${parentPipeline.id}_${parentPipelineStageId}_${pipelineConfig.id}_${parentPipeline.startTime}".toString(),
-      expectedArtifactIds  : expectedArtifactIds
+      expectedArtifactIds  : expectedArtifactIds.toSet().toList()


Why is this necessary?

only to avoid duplications between trigger and stages

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

dbyron-sf · 2023-07-20T15:57:41Z

It would also help to connect the dots to show if this ever worked in previous versions of spinnaker, what PR broke it, and ideally why.

…en trigger is pipeline type

nemesisOsorio · 2023-07-20T16:06:55Z

It would also help to connect the dots to show if this ever worked in previous versions of spinnaker, what PR broke it, and ideally why.

Absolutely, I tested it out in 1.28 and 1.27 and it was working
There is a chance the intention for expectedArtifacts was never to use them in stages like Deploy (Manifest), but right now, you can select an expectedArtifacts in the UI.

These are the related PRs
#4322
#4397
#4404

dbyron-sf · 2023-07-20T18:57:42Z

It would also help to connect the dots to show if this ever worked in previous versions of spinnaker, what PR broke it, and ideally why.

Absolutely, I tested it out in 1.28 and 1.27 and it was working There is a chance the intention for expectedArtifacts was never to use them in stages like Deploy (Manifest), but right now, you can select an expectedArtifacts in the UI.

These are the related PRs #4322 #4397 #4404

Great. Those PRs look like they went in to 1.30, so I assume it's been broken since then. Any idea about 1.29?

dbyron-sf · 2023-07-20T18:58:21Z

Also, cc: @jervi since you made those PRs.

nemesisOsorio · 2023-07-20T19:34:10Z

It would also help to connect the dots to show if this ever worked in previous versions of spinnaker, what PR broke it, and ideally why.

Absolutely, I tested it out in 1.28 and 1.27 and it was working There is a chance the intention for expectedArtifacts was never to use them in stages like Deploy (Manifest), but right now, you can select an expectedArtifacts in the UI.
These are the related PRs #4322 #4397 #4404

Great. Those PRs look like they went in to 1.30, so I assume it's been broken since then. Any idea about 1.29?

I tested it out in orca release-1.29.x and it is working, so you are right it's been broken since 1.30

edgarulg

Overall these changes looks good to me. This is an issue present in 1.30 & 1.31 only. I left my approve but I want @dbyron-sf or @xibz to have last word on approve these changes.

I also added @jervi as reviewer because these seems to be related with his PRs.

edgarulg · 2023-07-20T20:06:52Z

Also I'm seeing the build failing so would be good to rerun it because seems to be a false-positive error in the tests.

dbyron-sf · 2023-07-20T23:13:06Z

@nemesisOsorio I'm still hoping for a bit more in the description I think. Something like

Before PR XYZ (i.e. in spinnaker < 1.30), when pipeline B contained a trigger on
the completion of pipeline A with an artifact constraint, pipeline B only
triggered when pipeline A provided that artifact via the XXX field in the
pipeline execution context.  If pipeline A didn't provide that artifact,
pipeline B wouldn't trigger.  After that PR, pipeline B wouldn't trigger either
way -- even if pipeline A provided that artifact.  This PR restores the pre-PR
XYZ behavior.

but with the important missing bits filled in.

jervi

Sorry for overlooking this artifact flow when creating the aforementioned PR's 😬
But I think this is a good fix @nemesisOsorio.

xibz · 2023-07-21T09:10:56Z

@nemesisOsorio Thank you for the changes and updates! This looks a lot better :).

Sorry for overlooking this artifact flow

@jervi No worries! It happens. Im trying to think of any major features I've written that contained 0 bugs. Yep, can't think of one haha.

dbyron-sf · 2023-07-21T15:23:55Z

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

mergify · 2023-07-21T15:24:00Z

backport release-1.30.x release-1.31.x

✅ Backports have been created

#4490 fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type (backport #4489) has been created for branch release-1.30.x
#4491 fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type (backport #4489) has been created for branch release-1.31.x

…en trigger is pipeline type (#4489) (cherry picked from commit 45de5bc)

…en trigger is pipeline type (#4489) (#4490) (cherry picked from commit 45de5bc) Co-authored-by: Nemesis Osorio <nemesis211_6@hotmail.com>

…en trigger is pipeline type (#4489) (#4491) (cherry picked from commit 45de5bc) Co-authored-by: Nemesis Osorio <nemesis211_6@hotmail.com>

…en if you have 2 or more of the same type (#4579) * refactor(artifacts): partially reverting #4397 * refactor(artifacts): reverted #4489 * refactor(artifacts): reverted #4526 * test(artifacts): added test when parent pipeline does not provide expected artifacts * test(artifacts): resolve artifacts for default and prior artifacts --------- Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com>

…en if you have 2 or more of the same type (#4579) * refactor(artifacts): partially reverting #4397 * refactor(artifacts): reverted #4489 * refactor(artifacts): reverted #4526 * test(artifacts): added test when parent pipeline does not provide expected artifacts * test(artifacts): resolve artifacts for default and prior artifacts --------- Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com> (cherry picked from commit 645059d) # Conflicts: # orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy

…en if you have 2 or more of the same type (backport #4579) (#4587) * fix(artifacts): Automated triggers with artifact constraints are broken if you have 2 or more of the same type (#4579) * refactor(artifacts): partially reverting #4397 * refactor(artifacts): reverted #4489 * refactor(artifacts): reverted #4526 * test(artifacts): added test when parent pipeline does not provide expected artifacts * test(artifacts): resolve artifacts for default and prior artifacts --------- Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com> (cherry picked from commit 645059d) # Conflicts: # orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy * fix(artifacts): resolving git conflicts from #4579 for release-1.31.x (#4590) --------- Co-authored-by: Nemesis Osorio <nemesis211_6@hotmail.com>

…en trigger is pipeline type (spinnaker#4489) (spinnaker#4490) (cherry picked from commit 45de5bc) Co-authored-by: Nemesis Osorio <nemesis211_6@hotmail.com>

nemesisOsorio mentioned this pull request Jul 19, 2023

fix(artifacts): Ignoring the artifacts filter for pipeline stage #4487

Closed

dbyron-sf reviewed Jul 20, 2023

View reviewed changes

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

fix(artifacts): consider requiredArtifactIds in expected artifacts wh…

21743b4

…en trigger is pipeline type

nemesisOsorio force-pushed the fix/required-artifacts branch from 4d000b1 to 21743b4 Compare July 20, 2023 15:58

edgarulg approved these changes Jul 20, 2023

View reviewed changes

edgarulg requested a review from jervi July 20, 2023 20:02

jervi approved these changes Jul 21, 2023

View reviewed changes

dbyron-sf approved these changes Jul 21, 2023

View reviewed changes

dbyron-sf added ready to merge Approved and ready for merge backport-candidate Add to PRs to designate release branch patch candidates. labels Jul 21, 2023

mergify bot added the auto merged Merged automatically by a bot label Jul 21, 2023

mergify bot merged commit 45de5bc into spinnaker:master Jul 21, 2023
4 checks passed

mergify bot pushed a commit that referenced this pull request Jul 21, 2023

fix(artifacts): consider requiredArtifactIds in expected artifacts wh…

6dad7a7

…en trigger is pipeline type (#4489) (cherry picked from commit 45de5bc)

mergify bot pushed a commit that referenced this pull request Jul 21, 2023

fix(artifacts): consider requiredArtifactIds in expected artifacts wh…

b3dd9f0

…en trigger is pipeline type (#4489) (cherry picked from commit 45de5bc)

This was referenced Jul 21, 2023

fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type (backport #4489) #4490

Merged

fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type (backport #4489) #4491

Merged

spinnakerbot added the target-release/1.32 label Jul 21, 2023

nemesisOsorio mentioned this pull request Jul 21, 2023

REQUEST: New Approver status for nemesisOsorio spinnaker/governance#318

Open

6 tasks

sergio-quintero mentioned this pull request Oct 17, 2023

fix(artifacts): Consider artifacts from stages when triggering from a pipeline stage #4568

Closed

nemesisOsorio mentioned this pull request Oct 24, 2023

fix(artifacts): separate resolve artifacts for trigger of type pipeline #4570

Closed

nemesisOsorio added a commit to armory-io/orca that referenced this pull request Oct 30, 2023

refactor(artifacts): reverted spinnaker#4489

99d5a1c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type #4489

fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type #4489

nemesisOsorio commented Jul 19, 2023 •

edited

Loading

dbyron-sf Jul 20, 2023

nemesisOsorio Jul 20, 2023

dbyron-sf commented Jul 20, 2023

nemesisOsorio commented Jul 20, 2023

dbyron-sf commented Jul 20, 2023

dbyron-sf commented Jul 20, 2023

nemesisOsorio commented Jul 20, 2023 •

edited

Loading

edgarulg left a comment

edgarulg commented Jul 20, 2023

dbyron-sf commented Jul 20, 2023

jervi left a comment

xibz commented Jul 21, 2023 •

edited

Loading

dbyron-sf commented Jul 21, 2023

mergify bot commented Jul 21, 2023 •

edited

Loading

fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type #4489

fix(artifacts): consider requiredArtifactIds in expected artifacts when trigger is pipeline type #4489

Conversation

nemesisOsorio commented Jul 19, 2023 • edited Loading

dbyron-sf Jul 20, 2023

Choose a reason for hiding this comment

nemesisOsorio Jul 20, 2023

Choose a reason for hiding this comment

dbyron-sf commented Jul 20, 2023

nemesisOsorio commented Jul 20, 2023

dbyron-sf commented Jul 20, 2023

dbyron-sf commented Jul 20, 2023

nemesisOsorio commented Jul 20, 2023 • edited Loading

edgarulg left a comment

Choose a reason for hiding this comment

edgarulg commented Jul 20, 2023

dbyron-sf commented Jul 20, 2023

jervi left a comment

Choose a reason for hiding this comment

xibz commented Jul 21, 2023 • edited Loading

dbyron-sf commented Jul 21, 2023

mergify bot commented Jul 21, 2023 • edited Loading

✅ Backports have been created

nemesisOsorio commented Jul 19, 2023 •

edited

Loading

nemesisOsorio commented Jul 20, 2023 •

edited

Loading

xibz commented Jul 21, 2023 •

edited

Loading

mergify bot commented Jul 21, 2023 •

edited

Loading