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

kubectl: remove usage info from bad flag msg, only print help tip #82423

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

sallyom
Copy link
Contributor

@sallyom sallyom commented Sep 6, 2019

/kind cleanup

What this PR does / why we need it:
kubectl cmds have many options/flags. When a user passes a bad flag, cmd should only output a tip to run --help, rather than print the usage info.

Which issue(s) this PR fixes:
Fixes #66572

Does this PR introduce a user-facing change?:

If a bad flag is supplied to a kubectl command, only a tip to run --help is printed, instead of the usage menu.  Usage menu is printed upon running `kubectl command --help`. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @sallyom. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 6, 2019
@sallyom
Copy link
Contributor Author

sallyom commented Sep 6, 2019

example with PR:

$ kubectl config --foo
Error: unknown flag: --foo
See 'kubectl config --help' for usage.
$ kubectl config view --foo
Error: unknown flag: --foo
See 'kubectl config view --help' for usage.

example without:

$ kubectl config --foo
Error: unknown flag: --foo


Available Commands:
  current-context Displays the current-context
  delete-cluster  Delete the specified cluster from the kubeconfig
  delete-context  Delete the specified context from the kubeconfig
  get-clusters    Display clusters defined in the kubeconfig
  get-contexts    Describe one or many contexts
  rename-context  Renames a context from the kubeconfig file.
  set             Sets an individual value in a kubeconfig file
  set-cluster     Sets a cluster entry in kubeconfig
  set-context     Sets a context entry in kubeconfig
  set-credentials Sets a user entry in kubeconfig
  unset           Unsets an individual value in a kubeconfig file
  use-context     Sets the current-context in a kubeconfig file
  view            Display merged kubeconfig settings or a specified kubeconfig file

Usage:
  kubectl config SUBCOMMAND [options]

Use "kubectl <command> --help" for more information about a given command.
Use "kubectl options" for a list of global command-line options (applies to all commands).
$ kubectl config view --foo
Error: unknown flag: --foo


Examples:
  # Show merged kubeconfig settings.
  kubectl config view
  
  # Show merged kubeconfig settings and raw certificate data.
  kubectl config view --raw
  
  # Get the password for the e2e user
  kubectl config view -o jsonpath='{.users[?(@.name == "e2e")].user.password}'

Options:
      --allow-missing-template-keys=true: If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats.
      --flatten=false: Flatten the resulting kubeconfig file into self-contained output (useful for creating portable kubeconfig files)
      --merge=true: Merge the full hierarchy of kubeconfig files
      --minify=false: Remove all information not used by current-context from the output
  -o, --output='yaml': Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file.
      --raw=false: Display raw byte data
      --template='': Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].

Usage:
  kubectl config view [flags] [options]

Use "kubectl options" for a list of global command-line options (applies to all commands).

@sallyom
Copy link
Contributor Author

sallyom commented Sep 6, 2019

whoops kubectl options has disappeared, will fix. fixed

noticed kubectl options --foo needs a custom FlagErrorMsg, since options doesn't/shouldn't return a help menu

$ kubectl options --foo
unknown flag: --foo
Run 'kubectl options' without flags.

@eloyekunle
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2019
@soltysh soltysh self-assigned this Sep 6, 2019
@sallyom
Copy link
Contributor Author

sallyom commented Sep 9, 2019

/test pull-kubernetes-integration

@@ -66,6 +66,29 @@ type templater struct {
Filtered []string
}

type FlagError struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd replace all of this with just:

func (templater *templater) FlagErrorFunc(exposedFlags ...string) func(*cobra.Command, error) error {
    return func(c *cobra.Command, err error) error {
        c.SilenceUsage = true
        return fmt.Errorf("%s\nSee '%s --help' for usage.", err, c.CommandPath())
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 yup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the above change-it's much better, but with only this change something bugs me:

$ kubectl options --foo
Error: unknown flag: --foo
See 'kubectl options --help' for usage.

but kubectl options --help returns nothing.
I added a special case for the options --badflag to get:

$ kubectl options --foo
Error: unknown flag: --foo
Run 'kubectl options' without flags.

@@ -52,12 +52,6 @@ const (
{{ if $visibleFlags.HasFlags}}{{trimRight (flagsUsages $visibleFlags)}}{{end}}{{ if $explicitlyExposedFlags.HasFlags}}{{ if $visibleFlags.HasFlags}}
{{end}}{{trimRight (flagsUsages $explicitlyExposedFlags)}}{{end}}

{{end}}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave these as is, this way when you apply the previous suggestion and run just kubectl you'll get nice info what kubectl is, w/o it you won't. Compare yours output with:

$ kubectl
kubectl controls the Kubernetes cluster manager.

 Find more information at: https://kubernetes.io/docs/reference/kubectl/overview/
...

yours is missing above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2019
@sallyom
Copy link
Contributor Author

sallyom commented Oct 2, 2019

thanks for the review @soltysh

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 8, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, soltysh

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2019
@k8s-ci-robot k8s-ci-robot merged commit ea4f785 into kubernetes:master Oct 8, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 8, 2019
ohsewon pushed a commit to ohsewon/kubernetes that referenced this pull request Oct 16, 2019
kubectl: remove usage info from bad flag msg, only print help tip
wking pushed a commit to wking/kubernetes that referenced this pull request Jul 21, 2020
kubectl: remove usage info from bad flag msg, only print help tip

Kubernetes-commit: ea4f785
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Behavior of commands with respect to errors and printing usage is inconsistent and reduces usability
4 participants