-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): Allow multiple custom decorators of the same type #582
feat(core): Allow multiple custom decorators of the same type #582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Wait a minute before merging this. I found a little thing I want to fix. I’ll push it soon. |
Fixed in 85a4ca1 |
We've run Igor with this PR merged internally in Schibsted for 9 days now. @gardleopard or maybe @ajordens can you please merge it? 🙏 |
@@ -45,4 +47,5 @@ public GenericArtifact(String type, String name, String version, String referenc | |||
private String type; | |||
private String version; | |||
private Map<String, String> metadata; | |||
private boolean decorated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will serialize this field as well and surface it to the pipeline execution. I had originally annotated it with @JsonIgnore
but I removed it again because I felt it wouldn't hurt to actually expose the field. I'm happy to re-add it if someone feels different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I found a use for the flag (in spinnaker/orca#3422), so now I'm not so happy to ignore it anymore 😎
Each matched decorator will add another artifact to the build. This allows us to for instance migrate to new artifact types without breaking backwards compatibility. Example: We have historically used the type `docker` for docker images, but newer versions of Spinnaker expects Docker images to be of the type `docker/image`. Now we can define both decorators at the same time.
…doubled the number of artifacts in the output. Added test and a new check.
ce81307
to
01f21d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were going to implement something like this and here it is! 🎉
@spinnaker/oss-reviewers PTAL |
I think @spinnaker/oss-approvers are the ones with rights to merge? |
Will only add artifacts that have been decorated in Igor (depends on the pending PR spinnaker/igor#582; but it is safe to merge this PR independent of that one)
Will only add artifacts that have been decorated in Igor (depends on the pending PR spinnaker/igor#582; but it is safe to merge this PR independent of that one)
Will only add artifacts that have been decorated in Igor (depends on the pending PR spinnaker/igor#582; but it is safe to merge this PR independent of that one)
…#3422) * Revert "fix(artifacts): don't output buildArtifacts as korkArtifacts (#3417)" This reverts commit df36cfe. * fix(artifacts): Revert #3417, but only add decorated artifacts Will only add artifacts that have been decorated in Igor (depends on the pending PR spinnaker/igor#582; but it is safe to merge this PR independent of that one) * Add test for checking decorated flag * Simplify the code a tiny bit * Trigger new build * Trigger new build * Trigger new build Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Each matched decorator will add another artifact to the build. This allows us to for instance migrate to new artifact types without breaking backwards compatibility. Example: We have historically used the type
docker
for docker images, but newer versions of Spinnaker expects Docker images to be of the typedocker/image
. Now we can define both decorators at the same time.