fix(artifacts): Be more lenient when filtering expected artifacts #4397

jervi · 2023-02-17T22:00:04Z

It is currently hard/impossible to resolve artifacts in pipelines that doesn't have any triggers defined by themselves, and are triggered by a pipeline stage in another pipeline. Previously it was possible to get it to work by editing the json of the pipeline or in the UI by adding artifacts in a temp trigger and then removing it (this would keep the expected artifacts around). When I introduced trigger specific artifact constraints in #4322, I made it a lot harder (if not impossible) to do this because no triggers are used and thus all expected artifacts are filtered out.

This PR will fix this by copying expected artifacts from the parent execution (only if triggered by a pipeline stage), thus avoiding the whole situation altogether. It will also not filter away any expected artifacts where useDefaultArtifact or usePriorArtifact is set to true, and it will bind artifacts defined inline in stages (so that you can match artifacts using regex and not only SpEL).

jervi · 2023-02-20T12:24:42Z

@dbyron-sf Do you think you can have a look at this? A little follow up on #4322 👍

dbyron-sf · 2023-02-21T00:49:28Z

orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java

+          .getResolvedArtifacts()
+          .stream()
+          .findFirst();
+    } catch (InvalidRequestException e) {


What is throwing InvalidRequestException that we might catch here?

ArtifactResolver.resolveExpectedArtifacts() throws this if it's unable to resolve the artifact. It's not something we care about, as most often it will probably not be matched.

dbyron-sf · 2023-02-21T00:51:13Z

I could use an example of a pipeline + trigger that doesn't work before this PR, but does work after. Especially if it's also one that worked before #4322. And then, is there a place to put that pipeline in a test? Maybe the first commit in this PR could be adding that test?

jervi · 2023-02-21T09:18:14Z

I have rebased and split the commits so that I add the tests in the first commit and the implementation code in the second. There are two new tests, plus a minor edit to an existing one to check that the filter doesn't remove artifacts with useDefaultArtifact set to true.

I could use an example of a pipeline + trigger

The whole point is to fix pipelines without any defined triggers, that are triggered by some other pipeline (using a pipeline stage). They will get an implicit trigger when being executed, but the pipeline config is (optionally) defined without any triggers at all.

jervi · 2023-02-21T12:23:49Z

To elaborate - You have a regular pipeline with a trigger and some expected artifacts. This pipeline has a bunch of pipeline stages that starts different pipelines in order. In one of these child pipelines (that doesn't have any triggers defined), you have a deploy manifest stage, with an inline artifact defined.

Previously, this would not work - it would try to download the file paas.*.yml literally.
With this PR the example in the image will work. It will compare the inline artifact with artifacts from the parent pipeline and bind it successfully, without needing to resort to SPeL expressions.

dbyron-sf · 2023-02-21T18:01:18Z

Thanks for the detail, and splitting the commits (nit, the first commit is now test(artifacts) not fix(artifacts)). Would you mind confirming one more thing -- whether the pipeline you describe here worked before #4322?

jervi · 2023-02-21T19:23:55Z

Not directly - defined inline it would not have worked earlier. But what you could do before was to define the expected artifact(s) on the child pipeline (as a workaround), and then use that artifact instead of defining it inline. But that is kind of hackish - you need to add a trigger to be able to define the artifact, and then you can remove the trigger again. After #4322 any expected artifacts defined using this workaround would be ignored, since Orca now only considers artifacts on the trigger actually used in the execution.
The workaround made more sense with the legacy/old artifact support - then expected artifacts weren't connected to a trigger.

It is currently hard/impossible to resolve artifacts in pipelines that doesn't have any triggers defined by themselves, and are triggered by a pipeline stage in another pipeline. Previously it was possible to get it to work by editing the json of the pipeline or in the UI by adding artifacts in a temp trigger and then removing it (this would keep the expected artifacts around). When I introduced trigger specific artifact constraints in spinnaker#4322, I made it a lot harder (if not impossible) to do this because no triggers are used and thus all expected artifacts are filtered out. This commit contains tests that demonstrate the issue.

It is currently hard/impossible to resolve artifacts in pipelines that doesn't have any triggers defined by themselves, and are triggered by a pipeline stage in another pipeline. Previously it was possible to get it to work by editing the json of the pipeline or in the UI by adding artifacts in a temp trigger and then removing it (this would keep the expected artifacts around). When I introduced trigger specific artifact constraints in spinnaker#4322, I made it a lot harder (if not impossible) to do this because no triggers are used and thus all expected artifacts are filtered out. This commit contains implementation code to fix the tests added in the previous commit. It will fix the issue by copying expected artifacts from the parent execution (only if triggered by a pipeline stage), thus avoiding the whole situation altogether. It will also not filter away any expected artifacts where `useDefaultArtifact` or `usePriorArtifact` is set to `true`, and it will bind artifacts defined inline in stages (so that you can match artifacts using regex and not only SpEL).

dbyron-sf · 2023-02-23T21:23:29Z

Thanks much @jervi . Think any of this warrants a release note?

Explain the changes from spinnaker/orca#4397

jervi · 2023-02-24T09:33:27Z

@dbyron-sf I made a PR for some release notes: spinnaker/spinnaker.io#308

* docs(releasenotes): Changes to artifact handling Explain the changes from spinnaker/orca#4397 * Add image to repo

dbyron-sf · 2023-02-24T15:54:25Z

Thank you!

This commit partially reverts spinnaker#4397. It broke some pipelines for a team that had a CI stage that created more artifacts with the same name as the expected artifact from the original trigger, and then had a pipeline stage. The result was that the child pipeline was triggered with multiple artifacts with the same name (but different version numbers), and then failed to start because the artifact resolver matched multiple artifacts instead of exactly one. Turns out the changes in DependentPipelineStarter wasn't really needed to fix the issue that spinnaker#4397 tried to solve, so I'm reverting them.

…4404) This commit partially reverts #4397. It broke some pipelines for a team that had a CI stage that created more artifacts with the same name as the expected artifact from the original trigger, and then had a pipeline stage. The result was that the child pipeline was triggered with multiple artifacts with the same name (but different version numbers), and then failed to start because the artifact resolver matched multiple artifacts instead of exactly one. Turns out the changes in DependentPipelineStarter wasn't really needed to fix the issue that #4397 tried to solve, so I'm reverting them.

…innaker#4397) * test(artifacts): Be more lenient when filtering expected artifacts It is currently hard/impossible to resolve artifacts in pipelines that doesn't have any triggers defined by themselves, and are triggered by a pipeline stage in another pipeline. Previously it was possible to get it to work by editing the json of the pipeline or in the UI by adding artifacts in a temp trigger and then removing it (this would keep the expected artifacts around). When I introduced trigger specific artifact constraints in spinnaker#4322, I made it a lot harder (if not impossible) to do this because no triggers are used and thus all expected artifacts are filtered out. This commit contains tests that demonstrate the issue. * fix(artifacts): Be more lenient when filtering expected artifacts It is currently hard/impossible to resolve artifacts in pipelines that doesn't have any triggers defined by themselves, and are triggered by a pipeline stage in another pipeline. Previously it was possible to get it to work by editing the json of the pipeline or in the UI by adding artifacts in a temp trigger and then removing it (this would keep the expected artifacts around). When I introduced trigger specific artifact constraints in spinnaker#4322, I made it a lot harder (if not impossible) to do this because no triggers are used and thus all expected artifacts are filtered out. This commit contains implementation code to fix the tests added in the previous commit. It will fix the issue by copying expected artifacts from the parent execution (only if triggered by a pipeline stage), thus avoiding the whole situation altogether. It will also not filter away any expected artifacts where `useDefaultArtifact` or `usePriorArtifact` is set to `true`, and it will bind artifacts defined inline in stages (so that you can match artifacts using regex and not only SpEL).

…pinnaker#4404) This commit partially reverts spinnaker#4397. It broke some pipelines for a team that had a CI stage that created more artifacts with the same name as the expected artifact from the original trigger, and then had a pipeline stage. The result was that the child pipeline was triggered with multiple artifacts with the same name (but different version numbers), and then failed to start because the artifact resolver matched multiple artifacts instead of exactly one. Turns out the changes in DependentPipelineStarter wasn't really needed to fix the issue that spinnaker#4397 tried to solve, so I'm reverting them.

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, it 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 <benjamin_j_powell@apple.com> Signed-off-by: benjamin-j-powell <bjp@apple.com>

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, it 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>

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>

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>

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>

…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>

#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

…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>

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

jervi marked this pull request as draft February 17, 2023 22:08

jervi marked this pull request as ready for review February 20, 2023 12:21

dbyron-sf reviewed Feb 21, 2023

View reviewed changes

jervi force-pushed the more_lenient_filters_for_expected_artifacts branch from f07b693 to 28b1030 Compare February 21, 2023 09:11

jervi force-pushed the more_lenient_filters_for_expected_artifacts branch 2 times, most recently from f406928 to d8172fb Compare February 21, 2023 10:50

jervi added 2 commits February 21, 2023 20:30

jervi force-pushed the more_lenient_filters_for_expected_artifacts branch from d8172fb to 4816f8c Compare February 21, 2023 19:30

Merge branch 'master' into more_lenient_filters_for_expected_artifacts

97aa6b0

dbyron-sf approved these changes Feb 23, 2023

View reviewed changes

dbyron-sf added the ready to merge Approved and ready for merge label Feb 23, 2023

mergify bot added the auto merged Merged automatically by a bot label Feb 23, 2023

mergify bot merged commit 8df68b7 into spinnaker:master Feb 23, 2023

spinnakerbot added the target-release/1.30 label Feb 23, 2023

jervi added a commit to jervi/spinnaker.io that referenced this pull request Feb 24, 2023

docs(releasenotes): Changes to artifact handling

0be1df7

Explain the changes from spinnaker/orca#4397

jervi mentioned this pull request Feb 24, 2023

docs(releasenotes): Changes to artifact handling spinnaker/spinnaker.io#308

Merged

jervi deleted the more_lenient_filters_for_expected_artifacts branch February 24, 2023 09:33

dbyron-sf pushed a commit to spinnaker/spinnaker.io that referenced this pull request Feb 24, 2023

docs(releasenotes): Changes to artifact handling (#308)

80f2d57

* docs(releasenotes): Changes to artifact handling Explain the changes from spinnaker/orca#4397 * Add image to repo

jervi mentioned this pull request Mar 2, 2023

fix(artifacts): Stop copying expectedArtifactIds to child pipelines #4404

Merged

nemesisOsorio mentioned this pull request Jul 20, 2023

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

Merged

nemesisOsorio mentioned this pull request Oct 24, 2023

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

Closed

xibz mentioned this pull request Oct 30, 2023

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

Merged

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

refactor(artifacts): partially reverting spinnaker#4397

a2b5d51

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): Be more lenient when filtering expected artifacts #4397

fix(artifacts): Be more lenient when filtering expected artifacts #4397

jervi commented Feb 17, 2023 •

edited

Loading

jervi commented Feb 20, 2023

dbyron-sf Feb 21, 2023

jervi Feb 21, 2023

dbyron-sf commented Feb 21, 2023

jervi commented Feb 21, 2023 •

edited

Loading

jervi commented Feb 21, 2023

dbyron-sf commented Feb 21, 2023

jervi commented Feb 21, 2023

dbyron-sf commented Feb 23, 2023

jervi commented Feb 24, 2023

dbyron-sf commented Feb 24, 2023

fix(artifacts): Be more lenient when filtering expected artifacts #4397

fix(artifacts): Be more lenient when filtering expected artifacts #4397

Conversation

jervi commented Feb 17, 2023 • edited Loading

jervi commented Feb 20, 2023

dbyron-sf Feb 21, 2023

Choose a reason for hiding this comment

jervi Feb 21, 2023

Choose a reason for hiding this comment

dbyron-sf commented Feb 21, 2023

jervi commented Feb 21, 2023 • edited Loading

jervi commented Feb 21, 2023

dbyron-sf commented Feb 21, 2023

jervi commented Feb 21, 2023

dbyron-sf commented Feb 23, 2023

jervi commented Feb 24, 2023

dbyron-sf commented Feb 24, 2023

jervi commented Feb 17, 2023 •

edited

Loading

jervi commented Feb 21, 2023 •

edited

Loading