-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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-4578: migrate experimental-initial-corrupt-check flag to feature gate. #18478
Conversation
Hi @liangyuanpeng. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
f2d76f5
to
54a55bb
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files
... and 25 files with indirect coverage changes @@ Coverage Diff @@
## main #18478 +/- ##
==========================================
- Coverage 68.79% 68.78% -0.02%
==========================================
Files 420 420
Lines 35490 35489 -1
==========================================
- Hits 24416 24410 -6
+ Misses 9647 9644 -3
- Partials 1427 1435 +8 Continue to review full report in Codecov by Sentry.
|
/retest |
LGTM, please address the comments. |
54a55bb
to
f2cd9db
Compare
expectedFeatures: map[featuregate.Feature]bool{ | ||
features.StopGRPCServiceOnDefrag: false, | ||
features.DistributedTracing: false, | ||
features.InitialCorruptCheck: true, |
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.
Can you add a similar test case for setting --experimental-initial-corrupt-check
command line flag resulting in feature gate being flipped?
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.
Updated.
I can see that we have a test for ExperimentalFlags to FeatureGates
, this is the reason i have not add this test case here before.
etcd/server/embed/config_test.go
Line 782 in cb0cd4b
func TestSetFeatureGatesFromExperimentalFlags(t *testing.T) { |
Do we still need to add corresponding conversion tests here for each ExperimentalFlags?
f2cd9db
to
027c920
Compare
cc @ahrtr |
12b34fe
to
3bc83b9
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.
LGTM with a nit
Thanks
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, liangyuanpeng, serathius 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 |
Signed-off-by: Lan Liang <[email protected]>
3bc83b9
to
a966c07
Compare
To make it easier to add a new featuregate, I will clean up this test in the next PR |
/ok-to-test |
Sounds good. We should try to make the test more extensible. For example, have a list of experimental flags, and execute the set of test cases for each of them. Each time when we migrate an experimental flag, we just need to add it into the list. |
sure , it's what i want to do. |
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.
migrating experimental-initial-corrupt-check flag to feature gate.
part of #18023