From f78032fff99c60b75ae0c6b8887bbdc48025182e Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Wed, 30 Jan 2019 10:46:31 +0000 Subject: [PATCH] Ensure that plugins are only listed once in help outputs. They were listed twice in `docker --help` (but not `docker help`), since the stubs were added in both `tryRunPluginHelp` and the `setHelpFunc` closure. Calling `AddPluginStubCommands` earlier in `setHelpFunc` before the call to `tryRunPluginHelp` is sufficient. Also it is no longer necessary to add just valid plugins (`tryRunPluginHelp` handles invalid plugins correctly) so remove that logic (which was in any case broken for e.g. `docker --help`). Update the e2e test to check for duplicate entries and also to test `docker --help` which was previously missed. Signed-off-by: Ian Campbell --- cli-plugins/manager/cobra.go | 11 ++++------- cmd/docker/docker.go | 22 ++++++++++----------- e2e/cli-plugins/help_test.go | 38 ++++++++++++++++++++++++++++++------ 3 files changed, 47 insertions(+), 24 deletions(-) diff --git a/cli-plugins/manager/cobra.go b/cli-plugins/manager/cobra.go index 302d338a1c99..aae2cef6c20c 100644 --- a/cli-plugins/manager/cobra.go +++ b/cli-plugins/manager/cobra.go @@ -23,18 +23,15 @@ const ( CommandAnnotationPluginInvalid = "com.docker.cli.plugin-invalid" ) -// AddPluginCommandStubs adds a stub cobra.Commands for each plugin -// (optionally including invalid ones). The command stubs will have -// several annotations added, see `CommandAnnotationPlugin*`. -func AddPluginCommandStubs(dockerCli command.Cli, cmd *cobra.Command, includeInvalid bool) error { +// // AddPluginCommandStubs adds a stub cobra.Commands for each valid and invalid +// plugin. The command stubs will have several annotations added, see +// `CommandAnnotationPlugin*`. +func AddPluginCommandStubs(dockerCli command.Cli, cmd *cobra.Command) error { plugins, err := ListPlugins(dockerCli, cmd) if err != nil { return err } for _, p := range plugins { - if !includeInvalid && p.Err != nil { - continue - } vendor := p.Vendor if vendor == "" { vendor = "unknown" diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index ac7ef80eedd7..9302f7f7ce89 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -146,7 +146,6 @@ func setupHelpCommand(dockerCli *command.DockerCli, rootCmd, helpCmd *cobra.Comm func tryRunPluginHelp(dockerCli command.Cli, ccmd *cobra.Command, cargs []string) error { root := ccmd.Root() - pluginmanager.AddPluginCommandStubs(dockerCli, root, false) cmd, _, err := root.Traverse(cargs) if err != nil { @@ -169,6 +168,17 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag. ccmd.Println(err) return } + + // Add a stub entry for every plugin so they are + // included in the help output and so that + // `tryRunPluginHelp` can find them or if we fall + // through they will be included in the default help + // output. + if err := pluginmanager.AddPluginCommandStubs(dockerCli, ccmd.Root()); err != nil { + ccmd.Println(err) + return + } + if len(args) >= 1 { err := tryRunPluginHelp(dockerCli, ccmd, args) if err == nil { // Successfully ran the plugin @@ -189,16 +199,6 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag. return } - // Add a stub entry for every plugin so they are - // included in the help output. If we have no args - // then this is being used for `docker help` and we - // want to include broken plugins, otherwise this is - // `help «foo»` and we do not. - if err := pluginmanager.AddPluginCommandStubs(dockerCli, ccmd.Root(), len(args) == 0); err != nil { - ccmd.Println(err) - return - } - defaultHelpFunc(ccmd, args) }) } diff --git a/e2e/cli-plugins/help_test.go b/e2e/cli-plugins/help_test.go index f82959dbab05..2769c4c5470a 100644 --- a/e2e/cli-plugins/help_test.go +++ b/e2e/cli-plugins/help_test.go @@ -34,29 +34,55 @@ func TestGlobalHelp(t *testing.T) { // - The `badmeta` plugin under the "Invalid Plugins" heading. // // Regexps are needed because the width depends on `unix.TIOCGWINSZ` or similar. + helloworldre := regexp.MustCompile(`^ helloworld\s+\(Docker Inc\.\)\s+A basic Hello World plugin for tests$`) + badmetare := regexp.MustCompile(`^ badmeta\s+invalid metadata: invalid character 'i' looking for beginning of object key string$`) + var helloworldcount, badmetacount int for _, expected := range []*regexp.Regexp{ regexp.MustCompile(`^A self-sufficient runtime for containers$`), regexp.MustCompile(`^Management Commands:$`), regexp.MustCompile(`^ container\s+Manage containers$`), regexp.MustCompile(`^Commands:$`), regexp.MustCompile(`^ create\s+Create a new container$`), - regexp.MustCompile(`^ helloworld\s+\(Docker Inc\.\)\s+A basic Hello World plugin for tests$`), + helloworldre, regexp.MustCompile(`^ ps\s+List containers$`), regexp.MustCompile(`^Invalid Plugins:$`), - regexp.MustCompile(`^ badmeta\s+invalid metadata: invalid character 'i' looking for beginning of object key string$`), + badmetare, + nil, // scan to end of input rather than stopping at badmetare } { var found bool for scanner.Scan() { - if expected.MatchString(scanner.Text()) { + text := scanner.Text() + if helloworldre.MatchString(text) { + helloworldcount++ + } + if badmetare.MatchString(text) { + badmetacount++ + } + + if expected != nil && expected.MatchString(text) { found = true break } } - assert.Assert(t, found, "Did not find match for %q in `docker help` output", expected) + assert.Assert(t, expected == nil || found, "Did not find match for %q in `docker help` output", expected) } + // We successfully scanned all the input + assert.Assert(t, scanner.Scan() == false) + assert.NilError(t, scanner.Err()) + // Plugins should only be listed once. + assert.Assert(t, is.Equal(helloworldcount, 1)) + assert.Assert(t, is.Equal(badmetacount, 1)) + + // Running with `--help` should produce the same. + res2 := icmd.RunCmd(run("--help")) + res2.Assert(t, icmd.Expected{ + ExitCode: 0, + }) + assert.Assert(t, is.Equal(res2.Stdout(), res.Stdout())) + assert.Assert(t, is.Equal(res2.Stderr(), "")) - // Running just `docker` (without help) should produce the same thing, except on Stderr - res2 := icmd.RunCmd(run()) + // Running just `docker` (without `help` nor `--help`) should produce the same thing, except on Stderr. + res2 = icmd.RunCmd(run()) res2.Assert(t, icmd.Expected{ ExitCode: 0, })