-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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-3960: promote to beta in 1.30 #4367
Conversation
/cc @thockin |
And a draft code in k/k: kubernetes/kubernetes#122456 |
kindly ping @thockin |
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.
@kannon92 has one open
/lgtm
/approve
I am good with it - @kannon92 ? /lgtm |
Oh, right - this being sig-node I can't approve anyway. |
yes - will review later this week |
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 one small comment - other than that LGTM
Added in alpha: | ||
Unit tests in `k8s.io/kubernetes/pkg/apis/core/validation_test` test the verification with feature-gate enabled. | ||
The verification with feature-gate disabled is tested manually as it's hard to trigger `PrepareForCreate` strategy in unit tests. | ||
E2e tests in `test/e2e/common/node/lifecycle_hook.go` test the behavior with feature-gate enabled. |
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.
Copying from the KEP template:
Additionally, for features that are introducing a new API field, unit tests that
are exercising the `switch` of feature gate itself (what happens if I disable a
feature gate after having objects written with the new field) are also critical.
You can take a look at one potential example of such test in:
https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246-R282
I'm actually more interested in sth like that - this question is not about testing the feature itself (testing code with FG enabled), but rather testing the transition between enabled and disabled (and vice versa).
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.
... Yea, I think we need a test like this for this feature. But I clearly misunderstood this and missed this in alpha🤦.
I'll bring this test in beta, I added some cases in Unit tests
for beta.
cb25915
to
61ffb47
Compare
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AxeZhan, mrunalp, thockin, wojtek-t 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 |
/hold cancel |
Promote KEP-3960 to beta in 1.30
Works will be done for beta