-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ Handle variable definition conflicts for external variables #8107
✨ Handle variable definition conflicts for external variables #8107
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.
/hold
This PR contains a commit from #8076 which should be reviewed and merged first.
f5673c1
to
8a5d8b6
Compare
f07cbe2
to
042a2a6
Compare
This is ready for a round of review @sbueringer |
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.
Next iteration. And now reviewing internal/topology/variables
042a2a6
to
d12193b
Compare
2f79163
to
90f9126
Compare
Should be ready for another round @sbueringer |
/retest Some new flakes but independent of this PR (I'll probably take a look later) |
bdcc0e1
to
97b8caf
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry my bad. Have to fix the job config.. |
ea0f959
to
a9c7a35
Compare
a9c7a35
to
9ab052c
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.
This is outstanding work!
Just few comments from my side
func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterClassVariable *clusterv1.ClusterClassVariable, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) { | ||
if clusterVariable == nil { | ||
// defaultValue defaults a clusterVariable based on the default value in the clusterClassVariable. | ||
func defaultValue(currentValue *clusterv1.ClusterVariable, definition *statusVariableDefinition, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) { |
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.
kudos for cleaning up the wording (the distinction between value/definition make code more readable)
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.
Absolutely agree. clusterVariable vs clusterClassVariable was driving me slightly crazy :)
(disclaimer I wrote this code initially)
f1d4b91
to
30315cc
Compare
/hold remove Underlying PRs all merged |
/remove-hold |
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.
Last nits from my side
internal/topology/variables/cluster_variable_defaulting_test.go
Outdated
Show resolved
Hide resolved
internal/topology/variables/cluster_variable_defaulting_test.go
Outdated
Show resolved
Hide resolved
30315cc
to
56f9a52
Compare
Thank you very much!! Really great work, thank you especially for the extensive test coverage!!! /lgtm |
/assign @fabriziopandini |
LGTM label has been added. Git tree hash: 0265b8c4b59c34b7820460c203e763f106fba2f9
|
yay! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add handling for conflicting definitions in variables defined in the ClusterClass and external patches. This change includes:
DefinitionFrom
field when there are conflicting variablesPart of #7985