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 <ijc@docker.com>

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"

cmd/docker/docker.go

@@ -135,7 +135,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 {
@@ -155,6 +154,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
@@ -175,16 +185,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)
 	})
 }

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())
+	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 just `docker` (without help) should produce the same thing, except on Stderr
-	res2 := icmd.RunCmd(run())
+	// 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` nor `--help`) should produce the same thing, except on Stderr.
+	res2 = icmd.RunCmd(run())
 	res2.Assert(t, icmd.Expected{
 		ExitCode: 0,
 	})