fix(artifacts): Resolving is broken in multiple places #4526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xibz
force-pushed
the
broken-artifact-resolver
branch
5 times, most recently
from
59ec773
to
275b0aa
Compare
orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java
Outdated
Show resolved
Hide resolved
8df68b7 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 <[email protected]>
xibz
force-pushed
the
broken-artifact-resolver
branch
from
275b0aa
to
8d78af3
Compare
Signed-off-by: benjamin-j-powell <[email protected]>
xibz
force-pushed
the
broken-artifact-resolver
branch
from
8d78af3
to
d4898e3
Compare
nemesisOsorio
approved these changes
Sep 18, 2023
jervi
approved these changes
Sep 19, 2023
There was a problem hiding this comment.
I think this makes sense 😅 Thanks for fixing @xibz, and I'm sorry for introducing any breaking changes... The artifact resolving code is complicated - it's hard to introduce/fix anything there without inadvertently ruining someone else's workflows 🥴 (some of those workflows are being dependent on what I would call bugs in earlier Orca versions though)
|
@jervi awesome thank you! And no worries for the breaking changes; it happens :p especially as something as complex as the artifact resolving logic. One thing I plan on doing is documenting all my findings and knowledge in a README to help explain the whole complicated process, which may help facilitate discussion on the architecture. I do think it's overly complicated and there is room for improving the logic where we can simplify it, but that can be a separate discussion. So with that said, I'll ping you as well so we can have a group effort on updating this future README so we get everyone who's touched artifact resolving ensure that the knowledge gets dumped. |
dbyron-sf
reviewed
Sep 19, 2023
| // potentially return an empty list or null back, which will throw an | ||
| // exception. Before this commit, when getBoundArtifactForStage was called, | ||
| // the method would just retrieve the bound artifact from the stage context, | ||
| // and return it as an ExpectedArtifact type instead of a map. "Resolution" |
There was a problem hiding this comment.
Not sure I understand the instead of a map part. Looking at 8df68b7#diff-b25c50ecaceee2f8b0ae1ddc77fc92d48d16fb7ff9d15d27612685f1985b2a1eL136-R144, didn't both the "before" and "after" code return an Artifact?
This means I don't understand how getBoundArtifactForStage can return an empty list since the return type is Artifact.
I'm also a little unclear why there's a comment here, in resolveArtifacts about the behavior of getBoundArtifactForStage. I could use a bit more connecting of the dots.
xibz
force-pushed
the
broken-artifact-resolver
branch
from
48f31d1
to
3289317
Compare
Signed-off-by: benjamin-j-powell <[email protected]>
xibz
force-pushed
the
broken-artifact-resolver
branch
from
3289317
to
592570c
Compare
dbyron-sf
approved these changes
Sep 19, 2023
nemesisOsorio
added a commit
to armory-io/orca
that referenced
this pull request
Oct 30, 2023
mergify bot
pushed a commit
that referenced
this pull request
Nov 6, 2023
…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 <[email protected]>
mergify bot
pushed a commit
that referenced
this pull request
Nov 6, 2023
…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 <[email protected]> (cherry picked from commit 645059d) # Conflicts: # orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy
mergify bot
pushed a commit
that referenced
this pull request
Nov 6, 2023
…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 <[email protected]> (cherry picked from commit 645059d) # Conflicts: # orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy
mergify bot
pushed a commit
that referenced
this pull request
Nov 6, 2023
…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 <[email protected]> (cherry picked from commit 645059d) # Conflicts: # orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy
edgarulg
pushed a commit
that referenced
this pull request
Nov 7, 2023
…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 <[email protected]> (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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
8df68b7 broke custom triggers to Spinnaker integration. Prior to this commit if
getBoundArtifactForStagefailed to find a bound artifact, resolution would happen much later by utilizing thetriggersfield in stages. However,getBoundArtifactForStagenow throws an exception, which causes custom triggers to fail prematurely. Also if a custom trigger calls orchestrate directly and any pipeline containsgetBoundArtifactForStage, this will throw an exception and prevent any resolution.@jervi: please take a look at this as I noticed a couple of your PRs made some changes to artifact resolving, but I believe they only partially implemented the needed solution.