-
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
🐛 Fix ClusterClass variables status & RuntimeExtension and add test coverage #10337
🐛 Fix ClusterClass variables status & RuntimeExtension and add test coverage #10337
Conversation
/cherry-pick release-1.6 |
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…erage Co-authored-by: Stefan Büringer buringerst@vmware.com
4a48f60
to
6e70554
Compare
/test help |
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-cluster-api-e2e-main |
/lgtm /hold |
LGTM label has been added. Git tree hash: 7ebe7c3db66840fb2194f4bfcdd21774375c6391
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
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.
Nice! (just a note for a possible future improvement)
/lgtm
@@ -343,7 +345,7 @@ func addDefinitionToExistingStatusVariable(variable clusterv1.ClusterClassVariab | |||
// If definitions already conflict, no need to check. | |||
if !combinedVariable.DefinitionsConflict { | |||
currentDefinition := combinedVariable.Definitions[0] | |||
if !(currentDefinition.Required == newVariableDefinition.Required && reflect.DeepEqual(currentDefinition.Schema, newVariableDefinition.Schema)) { | |||
if !(currentDefinition.Required == newVariableDefinition.Required && reflect.DeepEqual(currentDefinition.Schema, newVariableDefinition.Schema) && reflect.DeepEqual(currentDefinition.Metadata, newVariableDefinition.Metadata)) { |
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.
note: I think this is ok for now, but in the future we can think about making this more tolerant to definitions with different metadata.
E.g. we merge metadata if there are no conflicting values
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.
That's a great idea.
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 intentionally made a different choice with the metadata we already have within the scheme (description, example, ...)
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 idea there was that we shouldn't merge something that might not be the same. Merging it could be risky if the semantic differs.
Verified that metadata shows up in status: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10337/pull-cluster-api-e2e-main/1773305962350776320/artifacts/clusters/bootstrap/resources/k8s-upgrade-with-runtimesdk-7ossxu/ClusterClass/quick-start-runtimesdk.yaml The e2e test failures have been caused by another change. This will be fixed in: #10340 /hold cancel |
/override pull-cluster-api-e2e-main See above |
@sbueringer: Overrode contexts on behalf of sbueringer: pull-cluster-api-e2e-main In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@sbueringer: #10337 failed to apply on top of branch "release-1.6":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
// +optional
to the metadataBackground:
Follow-up for:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
/area runtime-sdk