Skip to content
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

🌱 Use ClusterClass name index in ClusterClass webhook #5810

Merged

Conversation

killianmuldoon
Copy link
Contributor

@killianmuldoon killianmuldoon commented Dec 7, 2021

Signed-off-by: killianmuldoon [email protected]

Add the ClusterClass name index to the client call for getting a ClusterClass in the ClusterClass webhook.

Certain tests are moved to a new subpackage in order to run them with envtest. This is required as the fakeclient doesn't set up the indexes now in use by the ClusterClass webhook.

Note this PR includes duplicate files from #5803 and should not be merged until that PR is completed.

Fixes #5719

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 7, 2021
@killianmuldoon killianmuldoon changed the title 🌱use ClusterClass name index in ClusterClass webhook 🌱[WIP] use ClusterClass name index in ClusterClass webhook Dec 7, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2022
@killianmuldoon killianmuldoon changed the title 🌱[WIP] use ClusterClass name index in ClusterClass webhook 🌱 Use ClusterClass name index in ClusterClass webhook Feb 7, 2022
@killianmuldoon killianmuldoon changed the title 🌱 Use ClusterClass name index in ClusterClass webhook 🌱 WIP Use ClusterClass name index in ClusterClass webhook Feb 7, 2022
},
{
name: "error if many MachineDeploymentClasses, used in multiple Clusters using the modified ClusterClass, are removed",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests removed here as they no longer work after the move to using the index. Index requires envtest and these tests rely solely on fakeclient.

@killianmuldoon killianmuldoon changed the title 🌱 WIP Use ClusterClass name index in ClusterClass webhook 🌱 Use ClusterClass name index in ClusterClass webhook Feb 7, 2022
internal/webhooks/test/clusterclass_test.go Outdated Show resolved Hide resolved
internal/webhooks/test/clusterclass_test.go Outdated Show resolved Hide resolved
internal/webhooks/test/clusterclass_test.go Show resolved Hide resolved
internal/webhooks/test/clusterclass_test.go Outdated Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the webhook/cluster-index branch 2 times, most recently from d94cd51 to 4066f8a Compare February 11, 2022 16:37
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 11, 2022
@sbueringer
Copy link
Member

@killianmuldoon thx! lgtm pending squash

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2022
@k8s-ci-robot k8s-ci-robot merged commit cf849bb into kubernetes-sigs:main Feb 18, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add list by index to ClusterClass webhook
4 participants