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.

Sign up for GitHub

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

Jump to bottom

closes #2130 new command flag ErrorOnUnknownSubcommand #2167

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

closes #2130 new command flag ErrorOnUnknownSubcommand #2167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9892605
closes #2130 new command flag ErrorOnUnknownSubcommand
andremueller Jul 6, 2024
3974349
fix linting issue
andremueller Jul 8, 2024
133c649
moved to global EnableErrorOnUnknownSubcommand option
andremueller Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions cobra.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@
var finalizers []func()

const (
defaultPrefixMatching = false

Check failure on line 46 in cobra.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

File is not `gofmt`-ed with `-s` (gofmt)
defaultCommandSorting = true
defaultCaseInsensitive = false
defaultTraverseRunHooks = false
defaultErrorOnUnknownSubcommand = false
)

// EnablePrefixMatching allows setting automatic prefix matching. Automatic prefix matching can be a dangerous thing
Expand All @@ -65,6 +66,17 @@
// By default this is disabled, which means only the first run hook to be found is executed.
var EnableTraverseRunHooks = defaultTraverseRunHooks

// EnableErrorOnUnknownSubcommand controls the behavior of subcommand handling.
// When the flag is set true the behavior of Command.Execute() will change:
// If a sub-subcommand is not found ErrUnknownSubcommand will be returned on calling
// Command.Exec() instead of the old behavior where a nil error was sent.
// If the flag is false (default) the old behavior is performed.
// For this behavior the child subcommand must be nil.
// Example: in root/service/run - there would be an existing subcommand `run`
// in root/service/unknown - there would be an unknown subcommand `unknown` therefore returning an error
// `service` must have a nil Command.Run() function for this.
var EnableErrorOnUnknownSubcommand = defaultErrorOnUnknownSubcommand

// MousetrapHelpText enables an information splash screen on Windows
// if the CLI is started from explorer.exe.
// To disable the mousetrap, just set this variable to blank string ("").
Expand Down
11 changes: 9 additions & 2 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ type Group struct {
Title string
}

// ErrUnknownSubcommand is returned by Command.Execute() when the subcommand was not found.
// Hereto, the ErrorOnUnknownSubcommand flag must be set true.
var ErrUnknownSubcommand = errors.New("subcommand is unknown")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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]))


// Command is just that, a command for your application.
// E.g. 'go run ...' - 'run' is the command. Cobra requires
// you to define the usage and description as part of your command
Expand Down Expand Up @@ -923,9 +927,12 @@ func (c *Command) execute(a []string) (err error) {
}

if !c.Runnable() {
return flag.ErrHelp
if EnableErrorOnUnknownSubcommand {
return ErrUnknownSubcommand
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
	}

} else {
return flag.ErrHelp
}
}

c.preRun()

defer c.postRun()
Expand Down
97 changes: 97 additions & 0 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package cobra
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
Expand Down Expand Up @@ -174,6 +175,102 @@ func TestSubcommandExecuteC(t *testing.T) {
}
}

func TestSubcommandExecuteMissingSubcommand(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
const childName = "child"
const grandchildName = "grandchild"
EnableErrorOnUnknownSubcommand = false
defer func() { EnableErrorOnUnknownSubcommand = defaultErrorOnUnknownSubcommand }()
childCmd := &Command{Use: childName, Run: nil}
child2Cmd := &Command{Use: grandchildName, Run: emptyRun}
rootCmd.AddCommand(childCmd)
childCmd.AddCommand(child2Cmd)

// test existing command
c, output, err := executeCommandC(rootCmd, childName)
if !strings.HasPrefix(output, "Usage:") {
t.Errorf("Unexpected output: %v", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != childName {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// test existing sub command
c, output, err = executeCommandC(rootCmd, childName, grandchildName)
if output != "" {
t.Errorf("Unexpected output: %v", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != grandchildName {
t.Errorf(`invalid command returned from ExecuteC: expected "grandchild"', got: %q`, c.Name())
}

// now test a command which does not exist, we will get no error, just "Usage:" is printed
c, output, err = executeCommandC(rootCmd, childName, "unknownChild")
if !strings.HasPrefix(output, "Usage:") {
t.Errorf("Expected: 'Usage: ...'\nGot:\n %q\n", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != childName {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}
}

func TestSubcommandExecuteMissingSubcommandWithErrorOnUnknownSubcommand(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
const childName = "child"
const grandchildName = "grandchild"
EnableErrorOnUnknownSubcommand = true
defer func() { EnableErrorOnUnknownSubcommand = defaultErrorOnUnknownSubcommand }()
childCmd := &Command{Use: childName, Run: nil}
child2Cmd := &Command{Use: grandchildName, Run: emptyRun}
rootCmd.AddCommand(childCmd)
childCmd.AddCommand(child2Cmd)

// test existing command
c, output, err := executeCommandC(rootCmd, childName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if !strings.HasPrefix(output, "Error:") {
t.Errorf("Unexpected output: %v", output)
}
if !errors.Is(err, ErrUnknownSubcommand) {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != childName {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// test existing sub command
c, output, err = executeCommandC(rootCmd, childName, grandchildName)
if output != "" {
t.Errorf("Unexpected output: %v", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != grandchildName {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// now test a command which does not exist, we expect an error because of the ErrorOnUnknownSubcommand flag
c, output, err = executeCommandC(rootCmd, childName, "unknownChild")
if !strings.HasPrefix(output, "Error:") {
t.Errorf("Unexpected output: %v", output)
}
if !errors.Is(err, ErrUnknownSubcommand) {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != childName {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}
}

func TestExecuteContext(t *testing.T) {
ctx := context.TODO()

Expand Down
Loading