-
Notifications
You must be signed in to change notification settings - Fork 179
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
Add ScyllaDBCluster webhook validation #2271
Add ScyllaDBCluster webhook validation #2271
Conversation
@zimnx: GitHub didn't allow me to request PR reviews from the following users: zimnx. Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs. 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zimnx 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 |
6ae7f48
to
38ef9a7
Compare
/cc rzetelskik Ready to review |
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.
first batch
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 looks good to me on the high level, I really like thow the validating webhooks are structured in the codebase 👍.
Verifying for idempotency, nothing stands out as problematic to me.
@rzetelskik is it reasonable to defer the new checks you've requested (and possibly other wishlist checks) to a separate issue? I don't have enough context to have an opinion here. But having a PR stuck on scope expansion lacking bandwidth feels like a choice inferior to merging a "good enough" webhook.
lgtm
/assign rzetelskik
Which comments exactly are asking for a scope expansion? Unless I'm missing something, they either refer to missing (or incorrect) validation, or are nits, which can be ignored. |
@rzetelskik I meant specifically ReadinessGates validation, metadata validation, placement validation - looks like they ask that new checks be implemented by the author of the PR. I'm just floating an idea of merging a "good enough" webhook and deferring those to a separate issue. Note that I do not know if a webhook without those is "good enough". |
The thing with validation is that we cannot make it stricter between releases. Splitting it into followup issues could lead to forgeting about one. I'll add those validations as part of this effort. |
38ef9a7
to
5c458f7
Compare
5c458f7
to
21c08e4
Compare
21c08e4
to
32ac5b6
Compare
32ac5b6
to
99a26d1
Compare
/lgtm |
Description of your changes:
Adds validation rules for v1alpha1.ScyllaDBCluster to existing validating webhook.
Which issue is resolved by this Pull Request:
Resolves #2280