closes #2130 new command flag ErrorOnUnknownSubcommand

[andremueller]

Proposed a backward compatible solution to #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]

c is the current command here. Therefore I would need to set this new flag for every command throughout my program to get the new behaviour globally.

Do you feel this feature should be enabled per command or globally?

If globally, then you can add a global variable in cobra.go instead of in the command structure. Then the program can enable this feature in a single location.

[andremueller]

Hey there! You are right. I think the global behavior is more what I (and probably others) want! I will change that!

[andremueller]

Hi @marckhouzam. Did you have a chance to review my changes? Would the behavior be so ok?

[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)
	}
}

[marckhouzam]

Please run make lint to find the linter error.

[marckhouzam]

Let's format this command message like the one in legacyArgs() to have a consistent behaviour.
fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))

[marckhouzam]

Let's only return the error if there is at least one arg.
This will allow to get the usage printed without an error when running a valid command that doesn't have a Run. This is something users do a lot. For example running helm repo should print the usage without showing an error.

You should be able to move up the following lines to get the number of args:

argWoFlags := c.Flags().Args()
	if c.DisableFlagParsing {
		argWoFlags = a
	}

[marckhouzam]

This case should not show an error but should print the usage like before.

[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.