-
Notifications
You must be signed in to change notification settings - Fork 16
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
[ACM-10801] Added Validating Webhook for Discovery #231
[ACM-10801] Added Validating Webhook for Discovery #231
Conversation
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
/retest |
1 similar comment
/retest |
var _ webhook.Defaulter = &DiscoveredCluster{} | ||
|
||
// Default implements webhook.Defaulter so a webhook will be registered for the type | ||
func (r *DiscoveredCluster) Default() { |
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.
Is this doing anything?
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 just for setting the default
setting of the webhook. No harm in just having this log for the moment, since this func can be enhanced at a later time.
PR needs rebase. 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. |
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
discoveredclusterLog.Info("default", "Name", r.Name) | ||
} | ||
|
||
var _ webhook.Validator = &DiscoveredCluster{} |
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.
Why is there a global variable that's not defined?
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.
Following the kubebuilder
docs, they were adding that line when they were building the webhook
: https://book.kubebuilder.io/multiversion-tutorial/webhooks.html#and-maingo. I checked out a few other repos that are configuring a webhook and they are using the same logic, so it might be best for us to remain consistent with the other projects/components.
Signed-off-by: Disaiah Bennett <[email protected]>
Signed-off-by: Disaiah Bennett <[email protected]>
Quality Gate passedIssues Measures |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dislbenn, ngraham20 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 |
/override "KinD tests (1.21, latest)" |
@dislbenn: Overrode contexts on behalf of dislbenn: KinD tests (1.21, latest) 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. |
3643087
into
stolostron:main
Description
Created a validating webhook to prevent the
import-strategy
annotation from being added to non ROSA cluster.Related Issue
https://issues.redhat.com/browse/ACM-10801
Changes Made
Implemented a webhook to prevent the non ROSA clusters from being imported automatically.
Screenshots (if applicable)
Add screenshots or GIFs that demonstrate the changes visually, if relevant.
Checklist
Additional Notes
Add any additional notes, context, or information that might be helpful for reviewers.
Reviewers
Tag the appropriate reviewers who should review this pull request. To add reviewers, please add the following line:
/cc @reviewer1 @reviewer2
/cc @cameronmwall @ngraham20
Definition of Done