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

WIP Make Kubelet flags visible to docs generators #66034

Open
wants to merge 1 commit into
base: master
from

Conversation

@mtaufen
Contributor

mtaufen commented Jul 10, 2018

The Kubelet avoids using Cobra flag parsing to keep tight control of its
command line. We hadn't initially used the Cobra Command's flagset at
all, which meant many flags were missing from the generated Kubelet
docs. With this change, Kubelet's --help flag and the generated docs
report the same set of flags (verified with diff -b).

Edit: needs more work, this fix still allows docs generator to pick up global flags.

Fixes kubernetes/website#9413

NONE
Make Kubelet flags visible to docs generators
The Kubelet avoids using Cobra flag parsing to keep tight control of its
command line. We hadn't initially used the Cobra Command's flagset at
all, which meant many flags were missing from the generated Kubelet
docs. With this change, Kubelet's `--help` flag and the generated docs
report the same set of flags (verified with `diff -b`).

Fixes kubernetes/website#9413
@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jul 10, 2018

Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mtaufen
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yujuhong

If they are not already assigned, you can assign the PR to them by writing /assign @yujuhong in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Contributor

k8s-ci-robot commented Jul 10, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mtaufen
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yujuhong

If they are not already assigned, you can assign the PR to them by writing /assign @yujuhong in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

// This shouldn't affect flag parsing, since we disable Cobra's parsing
// via cmd.DisableFlagParsing, but we still make a copy in case something
// mutates the command's FlagSet under the hood in any code path (runtime or docs generation).
docsFlagSet := pflag.NewFlagSet(componentKubelet, pflag.ExitOnError)

This comment has been minimized.

@liggitt

liggitt Jul 10, 2018

Member

why are the docs picking up flags differently than the part of the command that generates kubelet --help? seems like the docs generator should be fixed, not make doc-specific changes within the command like this

@liggitt

liggitt Jul 10, 2018

Member

why are the docs picking up flags differently than the part of the command that generates kubelet --help? seems like the docs generator should be fixed, not make doc-specific changes within the command like this

@mtaufen

This comment has been minimized.

Show comment
Hide comment
@mtaufen

mtaufen Jul 10, 2018

Contributor

/hold

Aaaaaand even with this change, docs generators will incorrectly pick up any third-party flags the Kubelet doesn't register, because github.com/spf13/cobra/doc/md_docs.go:printOptions -> cmd.NonInheritedFlags -> cmd.LocalFlags -> cmd.mergePersistentFlags -> cmd.updateParentsPflags adds in the global command line...

"verified by diff -b" turned out to be a happy accident, the one unrecognized third-party flag is no longer linked in the Kubelet, and we got lucky

Contributor

mtaufen commented Jul 10, 2018

/hold

Aaaaaand even with this change, docs generators will incorrectly pick up any third-party flags the Kubelet doesn't register, because github.com/spf13/cobra/doc/md_docs.go:printOptions -> cmd.NonInheritedFlags -> cmd.LocalFlags -> cmd.mergePersistentFlags -> cmd.updateParentsPflags adds in the global command line...

"verified by diff -b" turned out to be a happy accident, the one unrecognized third-party flag is no longer linked in the Kubelet, and we got lucky

@mtaufen mtaufen changed the title from Make Kubelet flags visible to docs generators to WIP Make Kubelet flags visible to docs generators Jul 10, 2018

@Bradamant3

This comment has been minimized.

Show comment
Hide comment
@Bradamant3

Bradamant3 Jul 11, 2018

Member

@steveperry-53 @tengqm it seems we have some disagreement here and in kubernetes/website#9413 about where/how the fix should happen.

Why is the kubelet the outlier here? ./hack/generate-docs.sh does just fine against the other command-line tools ... I see ^^ that "The Kubelet avoids using Cobra flag parsing to keep tight control of its command line." But why the difference from other command-line tools? (Sorry, no time to go code diving atm ...)

Member

Bradamant3 commented Jul 11, 2018

@steveperry-53 @tengqm it seems we have some disagreement here and in kubernetes/website#9413 about where/how the fix should happen.

Why is the kubelet the outlier here? ./hack/generate-docs.sh does just fine against the other command-line tools ... I see ^^ that "The Kubelet avoids using Cobra flag parsing to keep tight control of its command line." But why the difference from other command-line tools? (Sorry, no time to go code diving atm ...)

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jul 11, 2018

Member

why the difference from other command-line tools?

because the kubelet is one of the few components that supports dynamic config, and has to manage the intersection of flags and config file as input to its runtime configuration.

Member

liggitt commented Jul 11, 2018

why the difference from other command-line tools?

because the kubelet is one of the few components that supports dynamic config, and has to manage the intersection of flags and config file as input to its runtime configuration.

@steveperry-53

This comment has been minimized.

Show comment
Hide comment
@steveperry-53

steveperry-53 Jul 11, 2018

Contributor

I'm going to defer to @tengqm and @liggitt on this one.

Contributor

steveperry-53 commented Jul 11, 2018

I'm going to defer to @tengqm and @liggitt on this one.

@Bradamant3

This comment has been minimized.

Show comment
Hide comment
@Bradamant3

Bradamant3 Jul 12, 2018

Member

Thanks @liggitt -- makes sense. And suggests that we do indeed need a different script to generate the kubelet docs. @tengqm do you have ideas? I am buried for at least the next week and a half, but can try to think about it after that if it would help. Happy to help brainstorm though if it can help.

Member

Bradamant3 commented Jul 12, 2018

Thanks @liggitt -- makes sense. And suggests that we do indeed need a different script to generate the kubelet docs. @tengqm do you have ideas? I am buried for at least the next week and a half, but can try to think about it after that if it would help. Happy to help brainstorm though if it can help.

@mtaufen

This comment has been minimized.

Show comment
Hide comment
@mtaufen

mtaufen Jul 17, 2018

Contributor
Contributor

mtaufen commented Jul 17, 2018

@mtaufen

This comment has been minimized.

Show comment
Hide comment
@mtaufen

mtaufen Jul 17, 2018

Contributor
Contributor

mtaufen commented Jul 17, 2018

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