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

fix(exit code): fix the return exit code 0 if subcommand excuted failed #8511

Closed

Conversation

donggangcj
Copy link

@donggangcj donggangcj commented Jul 24, 2020

In fact, this error would be better resolved in the dependency library cobra and i am also trying to fix this issue of cobra.

I read the code of other projects using cobra like kubectl , github cli and these project solved this problem.
spf13/cobra#1157

I think the way that kubectl used is better. kubectl adds Run function for those commands with subcommands:
https://github.com/kubernetes/kubernetes/blob/8a3fc2d7e2043bc980bb5301d2f4292020f1a2dd/staging/src/k8s.io/kubectl/pkg/cmd/get/get.go#L155-L170

Therefore, I add empty Run function for commands with subcommands.(dependency,get,plugin,repo,search,show).Everything is ok.

Close: #8286

Signed-off-by: Dong Gang dong.gang@daocloud.io

What this PR does / why we need it:
Fix #8286
Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Close: helm#8286

Signed-off-by: Dong Gang <dong.gang@daocloud.io>
@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 24, 2020
@donggangcj
Copy link
Author

Local test results:

# Before
➜  helm git:(fix-sub-command-exit-code-error) helm repo sadd foo https://foo/bar

This command consists of multiple subcommands to interact with chart repositories.

It can be used to add, remove, list, and index chart repositories.

Usage:
  helm repo [command]

Available Commands:
  add         add a chart repository
  index       generate an index file given a directory containing packaged charts
  list        list chart repositories
  remove      remove one or more chart repositories
  update      update information of available charts locally from chart repositories

Flags:
  -h, --help   help for repo

Global Flags:
      --add-dir-header                   If true, adds the file directory to the header
      --alsologtostderr                  log to standard error as well as files
      --debug                            enable verbose output
      --kube-apiserver string            the address and the port for the Kubernetes API server
      --kube-context string              name of the kubeconfig context to use
      --kube-token string                bearer token used for authentication
      --kubeconfig string                path to the kubeconfig file
      --log-backtrace-at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
      --log-dir string                   If non-empty, write log files in this directory
      ......

Use "helm repo [command] --help" for more information about a command.
➜  helm git:(fix-sub-command-exit-code-error) echo $?
0

# After
➜  helm git:(fix-sub-command-exit-code-error) bin/helm repo sadd foo https://foo/bar 
Error: "helm repo" accepts no arguments

Usage:  helm repo add|remove|list|index|update [ARGS] [flags]
➜  helm git:(fix-sub-command-exit-code-error) echo $?
1

Note, this pr will change the output from command.HelpFunc() to command.Long, I wonder if this will create other problems.

@bacongobbler
Copy link
Member

IMO, let's see if we can get a fix upstream first before we resort to hacks/workarounds.

@donggangcj
Copy link
Author

Indeed, we should give priority to get a fix upstream first .However, this seems to take a long time to wait.😁

@bacongobbler
Copy link
Member

bacongobbler commented Sep 23, 2020

If you wish to fix the issue, please do so upstream. We're happy to wait for a fix. Rushing to implement a hack or a workaround will only result in more issues down the line.

@donggangcj
Copy link
Author

@bacongobbler Okay, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto/go vulnerability found in helm v2.16.7
3 participants