Skip to content
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

Ensure etcd cli help flags cannot be missed #16034

Closed
jmhbnz opened this issue Jun 8, 2023 · 12 comments
Closed

Ensure etcd cli help flags cannot be missed #16034

jmhbnz opened this issue Jun 8, 2023 · 12 comments

Comments

@jmhbnz
Copy link
Member

jmhbnz commented Jun 8, 2023

What would you like to be added?

When we update etcd to introduce new command line parameters we currently don't have any tests or other automation to verify that our cli help stays in sync.

This situation played out recently where #14120 introduced new command line flags and reviewers did not catch the missing help content. This leads to followup pr's being required, example: #16031.

A potential approach we could try is introducing a test that will verify help content matches an expected template: kubernetes-sigs/metrics-server#939. Alternatively we could introduce some automation for the generation of the help content.

Why is this needed?

  • Ensure our help content is accurate and remains relevant.
  • Avoid user confusion and unnecessary support requests.
  • Reduce burden on pull request reviewers.
@charles-chenzz
Copy link

if I didn't understand it wrong, what we do here is add a file command-line-flags.txt and add command-line in etcd to the file, and refer the test-image.sh part of kubernetes-sigs/metrics-server#939

@jmhbnz

@jmhbnz
Copy link
Member Author

jmhbnz commented Jun 9, 2023

if I didn't understand it wrong, what we do here is add a file command-line-flags.txt and add command-line in etcd to the file, and refer the test-image.sh part of kubernetes-sigs/metrics-server#939

Hey @charles-chenzz - Yes I think we can re-use a similar approach. Rather than the bash script test though I'm thinking we could make it part of one the formal go test suites.

@serathius - If we go with implementing a test do you have any preference on if this is is a separate bash script approach, versus inclusion in one of the go test suites?

@charles-chenzz
Copy link

if we re-use similar approach I think I can take it up

@nitishchauhan0022
Copy link

nitishchauhan0022 commented Jun 9, 2023

The alternate approach of automated generation of help content sounds less error prone and will help the developers as well. If open for addition of that i can work on it.

@nitishchauhan0022
Copy link

hey @jmhbnz I am trying to understand the issue. One thing, i am not getting is if a developer forgets to add the help content of a new flag in server/etcdmain/help.go than how can verfiying it with a approach as mentioned in kubernetes-sigs/metrics-server#939 can detect this since the help content here is not generating itself but it is using server/etcdmain/help.go file. Please correct me if I'm mistaken, but these are my preliminary understandings at the moment.

@jmhbnz
Copy link
Member Author

jmhbnz commented Jun 9, 2023

hey @jmhbnz I am trying to understand the issue. One thing, i am not getting is if a developer forgets to add the help content of a new flag in server/etcdmain/help.go than how can verfiying it with a approach as mentioned in kubernetes-sigs/metrics-server#939 can detect this since the help content here is not generating itself but it is using server/etcdmain/help.go file. Please correct me if I'm mistaken, but these are my preliminary understandings at the moment.

Taking a closer look at this I think you're right - please feel free to propose an alternative idea based on automating the help content creation or more dynamically testing if it is up to date.

@riendeau
Copy link
Contributor

Would an approach like #16465 work?

@jmhbnz
Copy link
Member Author

jmhbnz commented Aug 23, 2023

Thanks for the great work on this @riendeau 🎉

@jmhbnz jmhbnz closed this as completed Aug 23, 2023
@serathius
Copy link
Member

Can we backport this to release-3.4 and release-3.5 to prevent issues like #17189 and #17190 ?

@ishan16696
Copy link
Contributor

Can we backport this to release-3.4 and release-3.5 to prevent issues like #17189 and #17190 ?

I can do this, feel free to assign me on this

@ishan16696
Copy link
Contributor

#16465 have been backported on branch release-3.4 and release-3.5. I guess we can close this issue now.

@serathius
Copy link
Member

Thanks @ishan16696 for quickly and thoroughly solving the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants