Merge pull request #1802 from ijc/only-parse-global-args-once

Only parse global args once

cli-plugins/plugin/plugin.go

@@ -40,10 +40,13 @@ func runPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
 		return err
 	}
 
-	cmd, _, err := tcmd.HandleGlobalFlags()
+	cmd, args, err := tcmd.HandleGlobalFlags()
 	if err != nil {
 		return err
 	}
+	// We've parsed global args already, so reset args to those
+	// which remain.
+	cmd.SetArgs(args)
 	return cmd.Execute()
 }
 

cmd/docker/docker.go

@@ -228,6 +228,9 @@ func runDocker(dockerCli *command.DockerCli) error {
 		}
 	}
 
+	// We've parsed global args already, so reset args to those
+	// which remain.
+	cmd.SetArgs(args)
 	return cmd.Execute()
 }
 

e2e/cli-plugins/flags_test.go

@@ -1,6 +1,7 @@
 package cliplugins
 
 import (
+	"os"
 	"testing"
 
 	"gotest.tools/icmd"
@@ -45,13 +46,13 @@ func TestClashWithGlobalArgs(t *testing.T) {
 			name:        "short-with-val",
 			args:        []string{"-c", "Christmas"},
 			expectedOut: "Merry Christmas!",
-			expectedErr: "",
+			expectedErr: icmd.None,
 		},
 		{
 			name:        "short-with-val",
 			args:        []string{"--context", "Christmas"},
 			expectedOut: "Merry Christmas!",
-			expectedErr: "",
+			expectedErr: icmd.None,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -66,6 +67,60 @@ func TestClashWithGlobalArgs(t *testing.T) {
 	}
 }
 
+// TestGlobalArgsOnlyParsedOnce checks that global args are only parsed
+// once (cf https://github.com/docker/cli/issues/1801). These tests
+// rely on `-H` being a list type (i.e. NewNamedListOptsRef) which
+// reject multiple uses dynamically (see `getServerHost()` in
+// github.com/docker/cli/cli/command/cli.go) in order to detect this
+// scenario.
+func TestGlobalArgsOnlyParsedOnce(t *testing.T) {
+	run, _, cleanup := prepare(t)
+	defer cleanup()
+
+	// We can rely on `$DOCKER_HOST` being set due to the call to
+	// `environment.Setup` in our `TestMain`.
+	dh := os.Getenv("DOCKER_HOST")
+
+	for _, tc := range []struct {
+		name                     string
+		args                     []string
+		expectedExitCode         int
+		expectedOut, expectedErr string
+	}{
+		{
+			// This is checking the precondition wrt -H mentioned in the function comment
+			name:             "fails-if-H-used-twice",
+			args:             []string{"-H", dh, "-H", dh, "version", "-f", "{{.Client.Version}}"},
+			expectedExitCode: 1,
+			expectedOut:      icmd.None,
+			expectedErr:      "Please specify only one -H",
+		},
+		{
+			name:             "builtin",
+			args:             []string{"-H", dh, "version", "-f", "{{.Client.Version}}"},
+			expectedExitCode: 0,
+			expectedOut:      "", // Will be the client version, but the specifics aren't important so long as stderr is empty.
+			expectedErr:      icmd.None,
+		},
+		{
+			name:             "plugin",
+			args:             []string{"-H", dh, "helloworld", "apiversion"},
+			expectedExitCode: 0,
+			expectedOut:      "", // Will be the client version, but the specifics aren't important so long as stderr is empty.
+			expectedErr:      icmd.None,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			res := icmd.RunCmd(run(tc.args...))
+			res.Assert(t, icmd.Expected{
+				ExitCode: tc.expectedExitCode,
+				Out:      tc.expectedOut,
+				Err:      tc.expectedErr,
+			})
+		})
+	}
+}
+
 // TestUnknownGlobal checks that unknown globals report errors
 func TestUnknownGlobal(t *testing.T) {
 	run, _, cleanup := prepare(t)

e2e/cli-plugins/main_test.go

@@ -0,0 +1,17 @@
+package cliplugins
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/docker/cli/internal/test/environment"
+)
+
+func TestMain(m *testing.M) {
+	if err := environment.Setup(); err != nil {
+		fmt.Println(err.Error())
+		os.Exit(3)
+	}
+	os.Exit(m.Run())
+}

e2e/cli-plugins/run_test.go

@@ -135,7 +135,7 @@ func TestHelpGood(t *testing.T) {
 	run, _, cleanup := prepare(t)
 	defer cleanup()
 
-	res := icmd.RunCmd(run("-D", "help", "helloworld"))
+	res := icmd.RunCmd(run("-l", "info", "help", "helloworld"))
 	res.Assert(t, icmd.Expected{
 		ExitCode: 0,
 		Err:      icmd.None,
@@ -150,7 +150,7 @@ func TestGoodHelp(t *testing.T) {
 	run, _, cleanup := prepare(t)
 	defer cleanup()
 
-	res := icmd.RunCmd(run("-D", "helloworld", "--help"))
+	res := icmd.RunCmd(run("-l", "info", "helloworld", "--help"))
 	res.Assert(t, icmd.Expected{
 		ExitCode: 0,
 		Err:      icmd.None,
@@ -159,7 +159,7 @@ func TestGoodHelp(t *testing.T) {
 	golden.Assert(t, res.Stdout(), "docker-help-helloworld.golden")
 	// Short -h should be the same, modulo the deprecation message
 	exp := shortHFlagDeprecated + res.Stdout()
-	res = icmd.RunCmd(run("-D", "helloworld", "-h"))
+	res = icmd.RunCmd(run("-l", "info", "helloworld", "-h"))
 	res.Assert(t, icmd.Expected{
 		ExitCode: 0,
 		// This should be identical to the --help case above
@@ -188,7 +188,7 @@ func TestHelpGoodSubcommand(t *testing.T) {
 	run, _, cleanup := prepare(t)
 	defer cleanup()
 
-	res := icmd.RunCmd(run("-D", "help", "helloworld", "goodbye"))
+	res := icmd.RunCmd(run("-l", "info", "help", "helloworld", "goodbye"))
 	res.Assert(t, icmd.Expected{
 		ExitCode: 0,
 		Err:      icmd.None,
@@ -203,7 +203,7 @@ func TestGoodSubcommandHelp(t *testing.T) {
 	run, _, cleanup := prepare(t)
 	defer cleanup()
 
-	res := icmd.RunCmd(run("-D", "helloworld", "goodbye", "--help"))
+	res := icmd.RunCmd(run("-l", "info", "helloworld", "goodbye", "--help"))
 	res.Assert(t, icmd.Expected{
 		ExitCode: 0,
 		Err:      icmd.None,