-
Notifications
You must be signed in to change notification settings - Fork 1k
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(provider/kubernetes): max-version-history annotation #2688
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.
The code LGTM, though I am a little surprised to see this instead of an implementation using the usual Spinnaker resource pointers (newest
, previous
, oldest
). Any reason not to take that approach?
@danielpeach we could think about implementing with resource pointers for more fine-grained control down the road, but that is largely infeasible when dealing with large bundles of resources, especially in cases where not all resources are submitted to a deploy stage each time. |
@@ -79,4 +79,7 @@ | |||
public static final String PAUSE_ROLLOUT_MANIFEST = "pauseRolloutManifest"; | |||
public static final String RESUME_ROLLOUT_MANIFEST = "resumeRolloutManifest"; | |||
public static final String UNDO_ROLLOUT_MANIFEST = "undoRolloutManifest"; | |||
|
|||
// Aritfact operations |
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.
Aritfact
-> Artifact
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.
👍
.collect(Collectors.toList()); | ||
|
||
KubernetesManifestStrategy strategy = KubernetesManifestAnnotater.getStrategy(manifest); | ||
if (strategy.getMaxVersionHistory() == null) { |
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.
Minor nit: It doesn't look like we need to execute lines 98 - 102 if strategy.getMaxVersionHistory() is null. This code could go at the start of the function to avoid unnecessary work.
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.
Good catch
Added to make cleanup of versioned resources simpler
a513044
to
5c28204
Compare
…2688) Added to make cleanup of versioned resources simpler
No description provided.