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
KEP-3716: graduate to stable for 1.30 #4435
Conversation
Skipping CI for Draft Pull Request. |
|
- [ ] Request in scope without `matchConditions`, and also matching | ||
- [ ] Multiple match conditions, covering the same cases as the single-condition case | ||
- [X] Feature gate enablement / disablement is a no-op when no `matchConditions` are set | ||
- [] Feature gate enablement / disablement works as expected when `matchConditions` are set |
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 test is a TODO I believe, we have one for testing the fields on the API object but not the functionality of the match conditions based on feature gate
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.
Good catch. This might not matter now since we remove the feature gate and the feature is always on when we graduate to GA. Still, this edit is correct. Maybe expand it with a comment that this got missed in Beta and that since we're now targeting GA we're about to delete the feature gate and don't need this?
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.
Just a couple small requests
- [ ] Request in scope without `matchConditions`, and also matching | ||
- [ ] Multiple match conditions, covering the same cases as the single-condition case | ||
- [X] Feature gate enablement / disablement is a no-op when no `matchConditions` are set | ||
- [] Feature gate enablement / disablement works as expected when `matchConditions` are set |
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.
Good catch. This might not matter now since we remove the feature gate and the feature is always on when we graduate to GA. Still, this edit is correct. Maybe expand it with a comment that this got missed in Beta and that since we're now targeting GA we're about to delete the feature gate and don't need this?
|
||
# The milestone at which this feature was, or is targeted to be, at each stage. | ||
milestone: | ||
alpha: "v1.27" | ||
beta: "v1.28" | ||
stable: "TBD" | ||
stable: "v1.30" |
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.
Would you also update keps/prod-readiness/sig-api-machinery/3716.yaml
and add a stable:
stanza?
GA requirements TBD | ||
<<[/UNRESOLVED]>> | ||
- Promote appropriate E2E tests to conformance | ||
- Cover any missing test coverage |
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 like we have:
Maybe throw the link into the KEP for other reviewers?
The test list is:
- "should be able to create and update validating webhook configurations with match conditions"
- "should be able to create and update mutating webhook configurations with match conditions"
- "should reject validating webhook configurations with invalid match conditions"
- "should reject mutating webhook configurations with invalid match conditions"
- "should mutate everything except 'skip-me' configmaps"
I spent a few minutes and couldn't think of anything else to add. I think they all should be promoted to conformance.
/label lead-opted-in |
_NON-BLOCKING for Alpha_ | ||
Details TBD. | ||
<<[/UNRESOLVED]>> | ||
The per call limit is shared with Validating Admission Policy CEL expressions and set at roughly 0.1 second for each expression evaluation call. The total budget per object (i.e. per ValidatingWebhook) for CEL match conditions is roughly .25 seconds and 1/4 the budget of Validating Admission Policy limit. |
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 reasonable to me.
cc @jpbetz
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.
SGTM as well
113079e
to
6c6b54e
Compare
6c6b54e
to
135106b
Compare
addressed comments |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, ivelichkovich 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 |
/lgtm looks like my comments have been addressed |
Updates testing/metrics information and stages KEP for GA in 1.30.