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.

Sign up for GitHub

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

Jump to bottom

✨ Add webhook warning for missing ClusterClass #8746

Merged
merged 1 commit into from
Jul 7, 2023
Merged

✨ Add webhook warning for missing ClusterClass #8746

merged 1 commit into from
Jul 7, 2023

Conversation

killianmuldoon
Copy link
Contributor

Add a warning in the Cluster webhook when the Cluster references a Cluster class which either does not exist or has not been successfully reconciled.

Warnings were added as a webhook feature in #8007.

Fixes #8496

/area clusterclass

@k8s-ci-robot k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2023
fabriziopandini
fabriziopandini reviewed May 29, 2023
View reviewed changes
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.

great to see this happening!
few suggestions about wording, not blocking

internal/webhooks/cluster.go Outdated Show resolved Hide resolved
internal/webhooks/cluster.go Outdated Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the pr-add-clusterclass-missing-warning branch from 681ddea to 8619336 Compare May 30, 2023 09:39
sbueringer
sbueringer reviewed May 30, 2023
View reviewed changes
internal/webhooks/cluster.go Show resolved Hide resolved
internal/webhooks/cluster_test.go Outdated Show resolved Hide resolved
@sbueringer sbueringer mentioned this pull request Jun 9, 2023
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023
@killianmuldoon killianmuldoon force-pushed the pr-add-clusterclass-missing-warning branch from 8619336 to 94e4093 Compare July 5, 2023 11:11
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2023
sbueringer
sbueringer reviewed Jul 6, 2023
View reviewed changes
internal/webhooks/cluster.go Outdated Show resolved Hide resolved
internal/webhooks/cluster.go Show resolved Hide resolved
vincepri
vincepri reviewed Jul 6, 2023
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

minor comments

internal/webhooks/cluster.go
case apierrors.IsNotFound(clusterClassPollErr):
allWarnings = append(allWarnings,
fmt.Sprintf(
"Cluster refers to ClusterClass %s in the topology but it does not exist. "+
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Cluster refers to ClusterClass %s in the topology but it does not exist. "+
"Cluster refers to ClusterClass %s in the topology but it does not exist (yet). "+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"yet" might be slightly misleading here if, for example, there's a typo in spec.topology.class

Copy link
Member

Choose a reason for hiding this comment

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

🤔 The suggestion here was for users that are installing CRDs all at once, and getting warnings because the ordering might be out of place, we might want to hint that things will reconcile eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This warning will occur whenever the ClsuterClass isn't found - so it might be because a user created everything at once or they didn't create a ClusterClass at all. I'd prefer to keep this warning more generic so it covers more of the likely cases.

Copy link
Member

@sbueringer sbueringer Jul 6, 2023 •
edited

Choose a reason for hiding this comment

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

Yup. I also prefer more generic warnings over misleading ones. The latter is more problematic

internal/webhooks/cluster.go Outdated Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the pr-add-clusterclass-missing-warning branch from 94e4093 to a8f764d Compare July 6, 2023 16:44
sbueringer
sbueringer reviewed Jul 6, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Thx for adding the additional tests!

internal/webhooks/cluster_test.go Outdated Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the pr-add-clusterclass-missing-warning branch from a8f764d to 697f0fa Compare July 7, 2023 08:06
@killianmuldoon
Add webhook warning for missing ClusterClass
32e5431 
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
@killianmuldoon killianmuldoon force-pushed the pr-add-clusterclass-missing-warning branch from 697f0fa to 32e5431 Compare July 7, 2023 08:07
@sbueringer
Copy link
Member

Thx!

/lgtm
/approve
/hold

Let's give others a bit of time to review again. If there are no objections until Monday I would say let's merge before code freeze.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2b795e4a4cf30d519858c01c1b5dc2a8ca065480

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2023
@fabriziopandini
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, 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:
  • OWNERS [fabriziopandini,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sbueringer
Copy link
Member

Given that all comments are resolved + we got a review from Fabrizio
Let's merge now

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit ad110b5 into kubernetes-sigs:main Jul 7, 2023
18 of 19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.5 milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackfrancis jackfrancis jackfrancis left review comments

@vincepri vincepri vincepri left review comments

@sbueringer sbueringer sbueringer left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@ykakarap ykakarap ykakarap left review comments

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterclass Issues or PRs related to clusterclass 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
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

Add webhook warning for Clusters without updated ClusterClass
7 participants
@killianmuldoon @sbueringer @k8s-ci-robot @fabriziopandini @jackfrancis @vincepri @ykakarap