-
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
🌱 Move Cluster and ClusterClass webhook implementation to top level package #5596
🌱 Move Cluster and ClusterClass webhook implementation to top level package #5596
Conversation
efdfb1d
to
d8db5ff
Compare
d8db5ff
to
c20f7d3
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.
Looks good overall. A few nits
lgtm +/- squash |
ff5bf1a
to
e448467
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.
only a tiny nit/question from my side, otherwise lgtm
…kage Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
e448467
to
d4576e5
Compare
Not sure about the long method receiver, I've never seen one >2 or maybe >3 characters. But not a blocker and I couldn't find any guidance/recommendations. /lgtm |
Agreed it's not ideal or common - but I do think there's a risk of confusion here because of the type name (But it's the only way that makes sense when importing webhooks.Cluster). |
/lgtm |
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.
/approve
@killianmuldoon
cluster-api/api/v1beta1/cluster_webhook.go
Lines 49 to 50 in 30b8f68
// Deprecated: This method is going to be removed in a next release. | |
func (c *Cluster) Default() { |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
Move the implementations of validation checks from the internal API package to the webhooks package. The webhooks package was previously calling the public methods in the API package which are deprecated and due to be removed in a future release.
Some details of the internal methods of the validation methods were changed in order to make this move.
Fixes #5594