closes #2130 new command flag ErrorOnUnknownSubcommand

[andremueller]

Proposed a backward compatible solution to https://github.com/spf13/cobra/issues/2130 Hereto a new flag ErrorOnUnknownSubcommand was added to the Command struct. Unit tests were added accordingly (TestSubcommandExecuteMissingSubcommand and TestSubcommandExecuteMissingSubcommandWithErrorOnUnknownSubcommand.

Fixes #1156 #2130

[CLAassistant]

All committers have signed the CLA.

[marckhouzam]

@andremueller Sorry for the delay but I had to find time to look into the many issues and PRs that tried to address this issue in the hopes of not missing anything.

Looking at #1056 and its PR #1068 there is a case we need to handle which is the additional help topics. An additional help topic is a command that doesn't have a Run()/RunE() and no children, or children that also don't have a Run()/RunE(). It is basically commands that don't do anything except print the help text. Look for IsAdditionalHelpTopicCommand() to see the exact definition.

We need to handle this case properly when EnableErrorOnUnknownSubcommand == true so that programs can use your new option even if they also have those help topics.

After giving it some thought I think we can handle everything in the same fashion, as long as we don't return an error when there are no arguments. I'll put comments in-line to clarify.

Here is a very small program that creates a program with commands and with help topics like so, to help you see what I mean:

     __ child -> grandchild    
    /
root 
    \_ topic -> subtopic

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	Use:   "prog",
	Short: "desc for root",
}

var child = &cobra.Command{
	Use:   "child",
	Short: "desc for child",
}

var grandchild = &cobra.Command{
	Use:   "grandchild",
	Short: "desc for grandchild",
	// This has a Run so it is a real command (not a help topic)
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("grandchild called")
	},
}

var topic = &cobra.Command{
	Use:   "topic",
	Short: "desc for topic",
	// command without a run and for which all children don't have a run, so it is a help topic
}

var subtopic = &cobra.Command{
	Use:   "subtopic",
	Short: "desc for subtopic",
	// leaf command without a run, so it is a help topic
}

func main() {
	cobra.EnableErrorOnUnknownSubcommand = true
	rootCmd.AddCommand(child, topic)
	child.AddCommand(grandchild)
	topic.AddCommand(subtopic)
	if err := rootCmd.Execute(); err != nil {
		os.Exit(1)
	}
}

[leoluz]

Thank you @andremueller for working on this PR. We, from the Argo CD team, are also interested in this feature.

[andremueller]

Hey @marckhouzam. I tried to include your proposals in the code. Actually one linting issue fixed which was not mine. However, I removed the common error "class" ErrUnknownSubcommand.

Would it be a good idea to refactor all fmt.Errorf("unknown command xx %s", ...) with fmt.Errorf("%w xx %s", ErrUnknownCommand, ....) such that the main function could check with if errors.Is(err, ErrUnknownCommand) in favor of if strings.HasPrefix(err.Error(), "unknown command" {...} ?

Hint: ErrUnknownCommand would be thrown in both cases: unknown command and unknown subcommand.

[nirs]

Would it be a good idea to refactor all fmt.Errorf("unknown command xx %s", ...) with fmt.Errorf("%w xx %s", ErrUnknownCommand, ....) such that the main function could check with if errors.Is(err, ErrUnknownCommand) in favor of if strings.HasPrefix(err.Error(), "unknown command" {...} ?

Yes, there is no sane way to handle fmt.Errorf("unknown command xx %s", ...). It should be used only for fatal error.

Maybe cobra needs a mechanism for handling unknown commands? this can be used to implement plugins.

[dcaldastp]

Any updates on this PR?

[andremueller]

Yes would be interesting! What do you think @marckhouzam? Is there more work to be done on upstream Cobra before this feature can be added - regarding this error handling issue? I think this is the main show stopper to this time.

[reneleonhardt]

Sad that it takes a year to fix this problem 😞