From a6e96c758e3d3b41a2a81146342344eb82d1d4ec Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 5 Jul 2024 00:57:07 +0200 Subject: [PATCH] cli: improve output and consistency for unknown (sub)commands Before this patch, output for invalid top-level and sub-commands differed. For top-level commands, the CLI would print an error-message and a suggestion to use `--help`. For missing *subcommands*, we would hit a different code-path, and different output, which includes full "usage" / "help" output. While it is a common convention to show usage output, and may have been a nice gesture when docker was still young and only had a few commands and options ("you did something wrong; here's an overview of what you can use"), that's no longer the case, and many commands have a _very_ long output. The result of this is that the error message, which is the relevant information in this case - "You mis-typed something" - is lost in the output, and hard to find (sometimes even requiring scrolling back). The output is also confusing, because it _looks_ like something ran successfully (most of the output is not about the error!). Even further; the suggested resolution (try `--help` to see the correct options) is rather redundant, because running teh command with `--help` produces _exactly_ the same output as was just showh, baring the error message. As a fun fact, due to the usage output being printed, the output even contains not one, but _two_ "call to actions"; - `See 'docker volume --help'.` (under the erro message) - `Run 'docker volume COMMAND --help' for more information on a command.` (under the usage output) In short; the output is too verbose, confusing, and doesn't provide a good UX. Let's reduce the output produced so that the focus is on the important information. This patch: - Changes the usage to the short-usage. - Changes the error-message to mention the _full_ command instead of only the command after `docker` (so `docker no-such-command` instead of `no-such-command`). - Prefixes the error message with the binary / root-command name (usually `docker:`); this is something we can still decide on, but it's a pattern we already use in some places. The motivation for this is that `docker` commands can often produce output that's a combination of output from the CLI itself, output from the daemon, and even output from the container. The `docker:` prefix helps to distinguish where the message originated from (the `docker` CLI in this case). - Adds an empty line between the error-message and the "call to action" (`Run 'docker volume --help'...` in the example below). This helps separating the error message ("unkown flag") from the call-to-action. Before this patch: Unknown top-level command: docker nosuchcommand foo docker: 'nosuchcommand' is not a docker command. See 'docker --help' Unknown sub-command: docker volume nosuchcommand foo Usage: docker volume COMMAND Manage volumes Commands: create Create a volume inspect Display detailed information on one or more volumes ls List volumes prune Remove unused local volumes rm Remove one or more volumes update Update a volume (cluster volumes only) Run 'docker volume COMMAND --help' for more information on a command. After this patch: Unknown top-level command: docker nosuchcommand foo docker: unknown command: docker nosuchcommand Run 'docker --help' for more information Unknown sub-command: docker volume nosuchcommand foo docker: unknown command: 'docker volume nosuchcommand' Usage: docker volume COMMAND Run 'docker volume --help' for more information Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/cobra.go | 2 +- cli/required.go | 15 ++++++++++++--- cmd/docker/docker.go | 2 +- cmd/docker/docker_test.go | 2 +- .../testdata/docker-badmeta-err.golden | 5 +++-- .../testdata/docker-nonexistent-err.golden | 5 +++-- 6 files changed, 21 insertions(+), 10 deletions(-) diff --git a/cli-plugins/manager/cobra.go b/cli-plugins/manager/cobra.go index feff8a8fd6f3..9d09018001ed 100644 --- a/cli-plugins/manager/cobra.go +++ b/cli-plugins/manager/cobra.go @@ -82,7 +82,7 @@ func AddPluginCommandStubs(dockerCli command.Cli, rootCmd *cobra.Command) (err e cmd.HelpFunc()(rootCmd, args) return nil } - return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", cmd.Name()) + return fmt.Errorf("docker: unknown command: docker %s\n\nRun 'docker --help' for more information", cmd.Name()) }, ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { // Delegate completion to plugin diff --git a/cli/required.go b/cli/required.go index 454e24761359..ba4e6e8efa5e 100644 --- a/cli/required.go +++ b/cli/required.go @@ -1,8 +1,6 @@ package cli import ( - "strings" - "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -14,7 +12,13 @@ func NoArgs(cmd *cobra.Command, args []string) error { } if cmd.HasSubCommands() { - return errors.Errorf("\n" + strings.TrimRight(cmd.UsageString(), "\n")) + return errors.Errorf( + "%[1]s: unknown command: %[2]s %[3]s\n\nUsage: %[4]s\n\nRun '%[2]s --help' for more information", + binName(cmd), + cmd.CommandPath(), + args[0], + cmd.UseLine(), + ) } return errors.Errorf( @@ -99,6 +103,11 @@ func ExactArgs(number int) cobra.PositionalArgs { } } +// binName returns the name of the binary / root command (usually 'docker'). +func binName(cmd *cobra.Command) string { + return cmd.Root().Name() +} + //nolint:unparam func pluralize(word string, number int) string { if number == 1 { diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 7825c7d35455..ca732180f1df 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -84,7 +84,7 @@ func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { if len(args) == 0 { return command.ShowHelp(dockerCli.Err())(cmd, args) } - return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", args[0]) + return fmt.Errorf("docker: unknown command: docker %s\n\nRun 'docker --help' for more information", args[0]) }, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return isSupported(cmd, dockerCli) diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index 8fc7ade3a44f..84ea272a20ec 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -66,7 +66,7 @@ func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) { func TestExitStatusForInvalidSubcommand(t *testing.T) { err := runCliCommand(t, nil, nil, "invalid") - assert.Check(t, is.ErrorContains(err, "docker: 'invalid' is not a docker command.")) + assert.Check(t, is.ErrorContains(err, "docker: 'docker invalid' is not a docker command.")) } func TestVersion(t *testing.T) { diff --git a/e2e/cli-plugins/testdata/docker-badmeta-err.golden b/e2e/cli-plugins/testdata/docker-badmeta-err.golden index df2344638af7..9342bdaf5518 100644 --- a/e2e/cli-plugins/testdata/docker-badmeta-err.golden +++ b/e2e/cli-plugins/testdata/docker-badmeta-err.golden @@ -1,2 +1,3 @@ -docker: 'badmeta' is not a docker command. -See 'docker --help' +docker: unknown command: docker badmeta + +Run 'docker --help' for more information diff --git a/e2e/cli-plugins/testdata/docker-nonexistent-err.golden b/e2e/cli-plugins/testdata/docker-nonexistent-err.golden index f2265f6b9ecd..12c26f5f0083 100644 --- a/e2e/cli-plugins/testdata/docker-nonexistent-err.golden +++ b/e2e/cli-plugins/testdata/docker-nonexistent-err.golden @@ -1,2 +1,3 @@ -docker: 'nonexistent' is not a docker command. -See 'docker --help' +docker: unknown command: docker nonexistent + +Run 'docker --help' for more information