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.
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
KEP-28: Transient parameters #1450
base: main
Are you sure you want to change the base?
KEP-28: Transient parameters #1450
Changes from 10 commits
d57eb62
07570ca
4a855ac
eb299ee
dc63cc4
ee6956b
b489294
810a1a5
f12d1b0
c137ce6
c6e357f
83d238e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 missing link from the parameter-to-its-plan the biggest drawback with approaches (1) and (3). Only (2) solves it but at the cost of introducing a plan specific (and thus non-reusable) resources.
I'm not sure this is true. A transient parameter still needs a
trigger
field with its corresponding plan value:We could use the
kudo update
command the same way:but the parameter is never persisted.
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.
Tbh, at the moment we don't have a parameter-to-plan mapping either. We may have a
trigger
field on the parameter, but the param can be used in any other plans as well, so I am not sure how strong our mapping here is anyway.The more I think about it, i'd rather get rid of the trigger and always specify plans explicitly. But that's a discussion for a different KEP.
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
trigger
is always there, it just defaults to deploy if not specified. And to be precise: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.
Yeah - the "default to
deploy
" is problematic as well.There is a relation missing:
I feel this relationship is flawed. If one parameter can be used in a template, and that template can be used in multiple plans, why does the parameter trigger a single plan?
One of the next things I'll KEP out is probably the "Define No-Plan Trigger" on parameters, because with the
BACKUP_NAME
parameter we get a problem:BACKUP_NAME
is used bybackup
plan andrestore
plan. If theBACKUP_NAME
parameter has thebackup
plan as the trigger, it can't be used together with therestore
plan - because specifying multiple parameters that trigger different plans is prevented by the 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.
Or maybe it's enough to say "If a plan is explicitly triggered, it doesn't matter what
trigger
attributes the parameters have" - that might work as well...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 guess, the predominant plan is still
deploy
. Additionally, an n:n mapping would be probably very confusing from the UX perspective. But I'm guessing here.True. But having a second
RESTORE_NAME
parameter seems like a small price to pay 🤷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 basically:
which makes it the most intuitive approach (at least for me). However, this also moves us further away from the declarative into YAML-y programming language territory.
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 other cons don't look bad but this means that we lose reusability of some resources. Maybe this isn't that bad because these resources aren't reusable in the first place (e.g. backup Job) but I still see this somewhat critical.
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.
Afaik the main justification for the
kudo plan trigger
command was always the ability to trigger a plan without a corresponding parameter change. Otherwise, the user might as well simplykudo update
the parameter directly. So we might want restrict thekudo plan trigger
command to only accept transient parameters (which are marked as such in the definition).However, this is probably an unnecessary restriction that won't allow a mix of persistent and transient parameters.
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.
Well, we do have a restriction that the changed parameters can not trigger multiple plans, right? So we already have a restriction there, although it does not distinguish between transient/persistent parameters. So I agree, I think we don't need an additional restriction here.