cobra icon indicating copy to clipboard operation
cobra copied to clipboard

return non-zero exit code upon non-runnable subcommand

Open donggangcj opened this issue 4 years ago • 15 comments

when excuting a root comand , cobra returns success on typos and exit code is 1.

➜  ~ helm notfoundsubcommand
Error: unknown command "notfoundsubcommand" for "helm"
Run 'helm --help' for usage.
➜  ~ echo $?
1

when excuting a subcommand ,cobra returns success on typos but exit code is 0.

➜  ~ 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
....

➜  ~ echo $?
0

This issue has come up before and this is related pr.

The related helm pr is here.

donggangcj avatar Jul 11 '20 07:07 donggangcj

/assign

donggangcj avatar Jul 11 '20 07:07 donggangcj

Ref #842.

umarcor avatar Jul 11 '20 12:07 umarcor

This issue is being marked as stale due to a long period of inactivity

github-actions[bot] avatar Sep 10 '20 00:09 github-actions[bot]

@donggangcj , Interestingly enough I am not able to repro this. I'm curious why Helm would be yielding different results.

serenity Cobra Testing Ground/1156 » cat main.go 
package main

import (
  "github.com/spf13/cobra"
  "fmt"
)

var (
  parent = &cobra.Command{
    Short: "issue 1156",
    Long: "testing for issue 1156",
  }
  childOne = &cobra.Command{
    Use: "childone",
  }
  childTwo = &cobra.Command{
    Use: "childtwo",
    Run: func(cmd *cobra.Command, args []string) {
      fmt.Println("hello 1156")
    },
  }
)

func init() {
  parent.AddCommand(childOne)
  childOne.AddCommand(childTwo)
}

func main() {
  parent.Execute()
}
serenity Cobra Testing Ground/1156 » go run ./main.go typochildone
Error: unknown command "typochildone" for ""
Run ' --help' for usage.
serenity Cobra Testing Ground/1156 » echo $?
0
serenity Cobra Testing Ground/1156 » go run ./main.go childone typochildtwo
Usage:
   childone [command]

Available Commands:
  childtwo    

Flags:
  -h, --help   help for childone

Use " childone [command] --help" for more information about a command.
serenity Cobra Testing Ground/1156 » echo $?
0

Upon further review. This behavior seems to be stemming from logic within Helm itself. https://github.com/helm/helm/blob/b0fdb5461fbc7e1b52f61613d3e7a22d85c7a794/cmd/helm/helm.go#L99

jharshman avatar Sep 24 '20 20:09 jharshman

@jharshman IIRC you need to specify a value for the Args field in your commands to trigger the problem.

marckhouzam avatar Sep 24 '20 20:09 marckhouzam

@marckhouzam I see, not sure it has anything to do with Args, but I am able to replicate the issue now. The root of this problem might just be that there are different error paths here...

error being dropped due to it being of type flag.ErrHelp:

=> 954:			if err == flag.ErrHelp {
   955:				cmd.HelpFunc()(cmd, args)
   956:				return cmd, nil
   957:			}

So basically you can invoke your root command incorrectly and receive an error whereas sub commands after a successful root command invocation run through the above and return no error.

jharshman avatar Sep 24 '20 22:09 jharshman

A relevant modification in #842 is precisely fixing these inconsistencies. For deciding when to show the help and when to add args/subcommands in the help of non-runnable subcommands invoked without args, these situations need to be detected and reworked. Unfortunately, #842 is blocked by #841.

https://github.com/mongodb/mongocli/issues/202#issuecomment-670422368

umarcor avatar Sep 25 '20 12:09 umarcor

This issue is being marked as stale due to a long period of inactivity

github-actions[bot] avatar Nov 25 '20 00:11 github-actions[bot]

this is still important to me (I was trying to trigger the bot,...)

colemickens avatar Dec 01 '20 03:12 colemickens

This issue is being marked as stale due to a long period of inactivity

github-actions[bot] avatar Feb 17 '21 00:02 github-actions[bot]

With the time since the original complaint I want to repro this again and make sure its still relevant. I think it is and there may even be a duplicate of this issue elsewhere but wanted to label this as part of the backlog cleanup (#1600)

johnSchnake avatar Mar 25 '22 16:03 johnSchnake

IIRC, has something to do with the legacyArgs() function is may be tricky to fix due to backwards-compatibility issue. But I agree it is poor behaviour and would be great to find a solution.

marckhouzam avatar Mar 25 '22 17:03 marckhouzam

Hello. I am also running into this. The issue happens for me when a subcommand is passed an arugment it does not understand. Instead of the program exiting with non-zero, it dumps the usage string and exits with 0.

Minimal working example:

package main

import (
    "errors"
    "flag"
    "os"

    "github.com/spf13/cobra"
)

var root = &cobra.Command{
    Use: "myCmd",
}

var subCmd = &cobra.Command{
    Use: "subCmd",
}

func main() {

    root.AddCommand(subCmd)
    
    err := root.Execute()
    if err != nil {
         os.Exit(1)
    }
    os.Exit(0)
}

Then build, go build and run myCmd subCmd notAValidArg. Usage will be printed (which in this case is just an empty string) and the exit value is 0. Compare that with myCmd notAValigArg which will exit with 1.

I believe if you just return the error instead of nil, this main will exit with 1 in both cases.

https://github.com/spf13/cobra/blob/fd865a44e3c48afeb6a6dbddadb8a5519173e029/command.go#L1074

So this:

		if errors.Is(err, flag.ErrHelp) {
			cmd.HelpFunc()(cmd, args)
			return cmd, err
		}

Just propagate the error back up to the caller. I can submit this as a PR, if you'd like.

glawler avatar Jul 23 '23 21:07 glawler

I’d have to look back at the details but the issue was that previous fixes have caused backwards compatibility issues.

marckhouzam avatar Jul 24 '23 01:07 marckhouzam

Since my issue was closed as a duplicate to this, I would like to try to raise this issue again.

I understand that changing the behavior could break backward compatibility, but is there some workaround to detect this case?

ldufresnegs avatar Sep 28 '23 11:09 ldufresnegs