Merge pull request #1020 from vdemeester/todo-tada

Handle some TODOs in tests

cli/command/cli_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/docker/docker/client"
 	"github.com/gotestyourself/gotestyourself/assert"
 	is "github.com/gotestyourself/gotestyourself/assert/cmp"
+	"github.com/gotestyourself/gotestyourself/env"
 	"github.com/gotestyourself/gotestyourself/fs"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
@@ -44,7 +45,7 @@ func TestNewAPIClientFromFlags(t *testing.T) {
 
 func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) {
 	customVersion := "v3.3.3"
-	defer patchEnvVariable(t, "DOCKER_API_VERSION", customVersion)()
+	defer env.Patch(t, "DOCKER_API_VERSION", customVersion)()
 
 	opts := &flags.CommonOptions{}
 	configFile := &configfile.ConfigFile{}
@@ -53,19 +54,6 @@ func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) {
 	assert.Check(t, is.Equal(customVersion, apiclient.ClientVersion()))
 }
 
-// TODO: use gotestyourself/env.Patch
-func patchEnvVariable(t *testing.T, key, value string) func() {
-	oldValue, ok := os.LookupEnv(key)
-	assert.NilError(t, os.Setenv(key, value))
-	return func() {
-		if !ok {
-			assert.NilError(t, os.Unsetenv(key))
-			return
-		}
-		assert.NilError(t, os.Setenv(key, oldValue))
-	}
-}
-
 type fakeClient struct {
 	client.Client
 	pingFunc   func() (types.Ping, error)
@@ -260,7 +248,7 @@ func TestOrchestratorSwitch(t *testing.T) {
 				version: defaultVersion,
 			}
 			if testcase.envOrchestrator != "" {
-				defer patchEnvVariable(t, "DOCKER_ORCHESTRATOR", testcase.envOrchestrator)()
+				defer env.Patch(t, "DOCKER_ORCHESTRATOR", testcase.envOrchestrator)()
 			}
 
 			cli := &DockerCli{client: apiclient, err: os.Stderr}

cli/command/container/run.go

@@ -124,9 +124,6 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
 	stdout, stderr := dockerCli.Out(), dockerCli.Err()
 	client := dockerCli.Client()
 
-	// TODO: pass this as an argument
-	cmdPath := "run"
-
 	warnOnOomKillDisable(*hostConfig, stderr)
 	warnOnLocalhostDNS(*hostConfig, stderr)
 
@@ -163,7 +160,7 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
 
 	createResponse, err := createContainer(ctx, dockerCli, containerConfig, &opts.createOptions)
 	if err != nil {
-		reportError(stderr, cmdPath, err.Error(), true)
+		reportError(stderr, "run", err.Error(), true)
 		return runStartContainerErr(err)
 	}
 	if opts.sigProxy {
@@ -209,7 +206,7 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
 			<-errCh
 		}
 
-		reportError(stderr, cmdPath, err.Error(), false)
+		reportError(stderr, "run", err.Error(), false)
 		if copts.autoRemove {
 			// wait container to be removed
 			<-statusChan

cli/command/plugin/install.go

@@ -124,9 +124,8 @@ func buildPullConfig(ctx context.Context, dockerCli command.Cli, opts pluginOpti
 		Disabled:              opts.disable,
 		AcceptAllPermissions:  opts.grantPerms,
 		AcceptPermissionsFunc: acceptPrivileges(dockerCli, opts.remote),
-		// TODO: Rename PrivilegeFunc, it has nothing to do with privileges
+		PrivilegeFunc:         registryAuthFunc,
-		PrivilegeFunc: registryAuthFunc,
+		Args:                  opts.args,
-		Args:          opts.args,
 	}
 	return options, nil
 }

cli/command/plugin/remove.go

@@ -40,7 +40,6 @@ func runRemove(dockerCli command.Cli, opts *rmOptions) error {
 
 	var errs cli.Errors
 	for _, name := range opts.plugins {
-		// TODO: pass names to api instead of making multiple api calls
 		if err := dockerCli.Client().PluginRemove(ctx, name, types.PluginRemoveOptions{Force: opts.force}); err != nil {
 			errs = append(errs, err)
 			continue

cli/command/system/info.go

@@ -176,21 +176,6 @@ func prettyPrintInfo(dockerCli command.Cli, info types.Info) error {
 		for _, lbl := range info.Labels {
 			fmt.Fprintln(dockerCli.Out(), " "+lbl)
 		}
-		// TODO: Engine labels with duplicate keys has been deprecated in 1.13 and will be error out
-		// after 3 release cycles (17.12). For now, a WARNING will be generated. The following will
-		// be removed eventually.
-		labelMap := map[string]string{}
-		for _, label := range info.Labels {
-			stringSlice := strings.SplitN(label, "=", 2)
-			if len(stringSlice) > 1 {
-				// If there is a conflict we will throw out a warning
-				if v, ok := labelMap[stringSlice[0]]; ok && v != stringSlice[1] {
-					fmt.Fprintln(dockerCli.Err(), "WARNING: labels with duplicate keys and conflicting values have been deprecated")
-					break
-				}
-				labelMap[stringSlice[0]] = stringSlice[1]
-			}
-		}
 	}
 
 	fmt.Fprintln(dockerCli.Out(), "Experimental:", info.ExperimentalBuild)

e2e/container/attach_test.go

@@ -1,6 +1,7 @@
 package container
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -19,8 +20,8 @@ func TestAttachExitCode(t *testing.T) {
 }
 
 func runBackgroundContainsWithExitCode(t *testing.T, exitcode int) string {
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "run", "-d", "-i", "--rm", fixtures.AlpineImage,
-		"docker run -d -i --rm %s sh -c 'read; exit %d'", fixtures.AlpineImage, exitcode))
+		"sh", "-c", fmt.Sprintf("read; exit %d", exitcode))
 	result.Assert(t, icmd.Success)
 	return strings.TrimSpace(result.Stdout())
 }

e2e/container/kill_test.go

@@ -32,17 +32,14 @@ func TestKillContainer(t *testing.T) {
 }
 
 func runBackgroundTop(t *testing.T) string {
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "top")
-		"docker run -d %s top", fixtures.AlpineImage))
 	result.Assert(t, icmd.Success)
 	return strings.TrimSpace(result.Stdout())
 }
 
 func containerStatus(t *testing.T, containerID, status string) func(poll.LogT) poll.Result {
 	return func(poll.LogT) poll.Result {
-		result := icmd.RunCmd(
+		result := icmd.RunCommand("docker", "inspect", "-f", "{{ .State.Status }}", containerID)
-			shell(t, "docker inspect -f '{{ .State.Status }}' %s", containerID),
-		)
 		result.Assert(t, icmd.Success)
 		actual := strings.TrimSpace(result.Stdout())
 		if actual == status {

e2e/container/run_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	"github.com/docker/cli/e2e/internal/fixtures"
-	shlex "github.com/flynn-archive/go-shlex"
 	"github.com/gotestyourself/gotestyourself/assert"
 	is "github.com/gotestyourself/gotestyourself/assert/cmp"
 	"github.com/gotestyourself/gotestyourself/golden"
@@ -17,8 +16,8 @@ const registryPrefix = "registry:5000"
 func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) {
 	image := createRemoteImage(t)
 
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "run", "--rm", image,
-		"docker run --rm %s echo this is output", image))
+		"echo", "this", "is", "output")
 
 	result.Assert(t, icmd.Success)
 	assert.Check(t, is.Equal("this is output\n", result.Stdout()))
@@ -54,10 +53,3 @@ func createRemoteImage(t *testing.T) string {
 	icmd.RunCommand("docker", "rmi", image).Assert(t, icmd.Success)
 	return image
 }
-
-// TODO: move to gotestyourself
-func shell(t *testing.T, format string, args ...interface{}) icmd.Cmd {
-	cmd, err := shlex.Split(fmt.Sprintf(format, args...))
-	assert.NilError(t, err)
-	return icmd.Cmd{Command: cmd}
-}

e2e/stack/remove_test.go

@@ -1,13 +1,10 @@
 package stack
 
 import (
-	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/docker/cli/internal/test/environment"
-	shlex "github.com/flynn-archive/go-shlex"
-	"github.com/gotestyourself/gotestyourself/assert"
 	"github.com/gotestyourself/gotestyourself/golden"
 	"github.com/gotestyourself/gotestyourself/icmd"
 	"github.com/gotestyourself/gotestyourself/poll"
@@ -20,7 +17,7 @@ func TestRemove(t *testing.T) {
 	deployFullStack(t, stackname)
 	defer cleanupFullStack(t, stackname)
 
-	result := icmd.RunCmd(shell(t, "docker stack rm %s", stackname))
+	result := icmd.RunCommand("docker", "stack", "rm", stackname)
 
 	result.Assert(t, icmd.Expected{Err: icmd.None})
 	golden.Assert(t, result.Stdout(), "stack-remove-success.golden")
@@ -28,8 +25,8 @@ func TestRemove(t *testing.T) {
 
 func deployFullStack(t *testing.T, stackname string) {
 	// TODO: this stack should have full options not minimal options
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "stack", "deploy",
-		"docker stack deploy --compose-file=./testdata/full-stack.yml %s", stackname))
+		"--compose-file=./testdata/full-stack.yml", stackname)
 	result.Assert(t, icmd.Success)
 
 	poll.WaitOn(t, taskCount(stackname, 2), pollSettings)
@@ -66,10 +63,3 @@ func taskCount(stackname string, expected int) func(t poll.LogT) poll.Result {
 func lines(out string) int {
 	return len(strings.Split(strings.TrimSpace(out), "\n"))
 }
-
-// TODO: move to gotestyourself
-func shell(t *testing.T, format string, args ...interface{}) icmd.Cmd {
-	cmd, err := shlex.Split(fmt.Sprintf(format, args...))
-	assert.NilError(t, err)
-	return icmd.Cmd{Command: cmd}
-}

vendor.conf

@@ -28,9 +28,7 @@ github.com/googleapis/gnostic e4f56557df6250e1945ee6854f181ce4e1c2c646
 github.com/gorilla/context v1.1
 github.com/gorilla/mux v1.1
 github.com/gotestyourself/gotestyourself cf3a5ab914a2efa8bc838d09f5918c1d44d02909
-# FIXME(vdemeester) try to deduplicate this with gojsonpointer
 github.com/go-openapi/jsonpointer 46af16f9f7b149af66e5d1bd010e3574dc06de98
-# FIXME(vdemeester) try to deduplicate this with gojsonreference
 github.com/go-openapi/jsonreference 13c6e3589ad90f49bd3e3bbe2c2cb3d7a4142272
 github.com/go-openapi/spec 6aced65f8501fe1217321abf0749d354824ba2ff
 github.com/go-openapi/swag 1d0bd113de87027671077d3c71eb3ac5d7dbba72