-
Notifications
You must be signed in to change notification settings - Fork 162
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: (cluster) Promotion tasks #3121
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3121 +/- ##
==========================================
+ Coverage 51.55% 51.79% +0.23%
==========================================
Files 288 291 +3
Lines 26188 26460 +272
==========================================
+ Hits 13502 13705 +203
- Misses 11960 12026 +66
- Partials 726 729 +3 ☔ View full report in Codecov by Sentry. |
242b4e6
to
ffc480b
Compare
84869c9
to
05224bf
Compare
@@ -0,0 +1,264 @@ | |||
package kargo |
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 wasn't introduced by this PR, but this internal/kargo
package has been bugging me for a while because I don't feel like the package has a clearly defined purpose. (Maybe it does, and I'm failing to see it.)
If I'm not wrong, there's certainly no need to untangle this knot to get this PR merged, but (again, only if I'm right), I wonder if there's any other place that might be appropriate for the new functionality added to the package, in particular NewPromotionBuilder()
.
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 only alternative I can think of is inventing a new promotion
package, but I do not really like this alternative either...
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 can put a pin in it. Fortunately there are only a couple packages like this that seem to have collected unrelated bits of functionality over time for lack of a better home.
if downstream.Spec.PromotionTemplate != nil && | ||
len(downstream.Spec.PromotionTemplate.Spec.Steps) == 0 { | ||
// Avoid creating a Promotion if the downstream Stage has no promotion | ||
// steps and is therefore a "control flow" Stage. | ||
continue | ||
} | ||
if err := s.createPromotionFn(ctx, &newPromo); err != nil { | ||
newPromo, err := kargo.NewPromotionBuilder(s.client).Build(ctx, downstream, freight.Name) |
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 isn't a new problem, but the PR's making me more aware of it...
It seems that now, as before, we build out a Promo's steps from the template in three places:
- Promote to Stage API endpoint
- Promote to downstream Stages API endpoint
- Auto-Promotion by the Stage reconciler
In all cases, a Promo's steps are defined when it's created.
I'll avoid the question of whether it's better to copy/expand steps when the Promo is created vs being done by the Promo reconciler when the Promo's started (because I could see it both ways and we may have to confront this eventually), but the weird side effect of the Promo reconciler not doing it is that you can't create a Promo with kubectl unless you defined all the steps yourself. There's no way to say: "This Stage, this piece of Freight, and let the controller fill in the steps."
Should we at least have the Promo reconciler build/expand steps just-in-time if a work on a Promo starts and its steps are empty?
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 is an excellent point, but I wonder if instead of doing this — wouldn't it be better to introduce a modifying webhook?
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.
I think I like that idea best, actually. In this case, we could drop the kargo.NewPromotionBuilder(s.client).Build()
from three locations it's in now and just let the mutating webhook handle for all cases? That sounds like a win.
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 would still need some sort of builder, I think. But the inflation would happen as a secondary step in a single place.
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.
Oh... I didn't mean drop the builder. Just drop three calls to it in favor of one.
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Co-authored-by: Mayursinh Sarvaiya <[email protected]> Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
2a1b8e2
to
911cc13
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.
🔥
Fixes: #2827
This pull request introduces the PromotionTask and ClusterPromotionTask resources to Kargo. These new resource types allow you to streamline Stage resources by bundling commonly shared promotion steps into reusable tasks, which can be referenced in promotion templates.
PromotionTask
exampleStage
exampleFull example for testing
https://gist.githubusercontent.com/hiddeco/df2da6a88ad1966642ed6ee94bad9787/raw/a886da867108c4ef970dec2b137e81f6a4b1f6fe/tasks.yaml