From afba3f7a604c1b8294ee9e93bbee1664a8d9213e Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 23 Apr 2018 14:13:52 +0200 Subject: [PATCH 1/3] Handle some TODOs in tests Use more gotestyourself for `env.Patch`, and `icmd.RunCommand` Signed-off-by: Vincent Demeester Upstream-commit: ae03dd7f46064d2a1a72dbc65f11135216ffc033 Component: cli --- components/cli/cli/command/cli_test.go | 18 +++--------------- components/cli/e2e/container/attach_test.go | 5 +++-- components/cli/e2e/container/kill_test.go | 7 ++----- components/cli/e2e/container/run_test.go | 12 ++---------- components/cli/e2e/stack/remove_test.go | 16 +++------------- 5 files changed, 13 insertions(+), 45 deletions(-) diff --git a/components/cli/cli/command/cli_test.go b/components/cli/cli/command/cli_test.go index 432e653fdc..d8ac647dfa 100644 --- a/components/cli/cli/command/cli_test.go +++ b/components/cli/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} diff --git a/components/cli/e2e/container/attach_test.go b/components/cli/e2e/container/attach_test.go index 3e7d0ed244..393d8cc81e 100644 --- a/components/cli/e2e/container/attach_test.go +++ b/components/cli/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, - "docker run -d -i --rm %s sh -c 'read; exit %d'", fixtures.AlpineImage, exitcode)) + result := icmd.RunCommand("docker", "run", "-d", "-i", "--rm", fixtures.AlpineImage, + "sh", "-c", fmt.Sprintf("read; exit %d", exitcode)) result.Assert(t, icmd.Success) return strings.TrimSpace(result.Stdout()) } diff --git a/components/cli/e2e/container/kill_test.go b/components/cli/e2e/container/kill_test.go index 25a158c113..cfa0193867 100644 --- a/components/cli/e2e/container/kill_test.go +++ b/components/cli/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, - "docker run -d %s top", fixtures.AlpineImage)) + result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "top") 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( - shell(t, "docker inspect -f '{{ .State.Status }}' %s", containerID), - ) + result := icmd.RunCommand("docker", "inspect", "-f", "{{ .State.Status }}", containerID) result.Assert(t, icmd.Success) actual := strings.TrimSpace(result.Stdout()) if actual == status { diff --git a/components/cli/e2e/container/run_test.go b/components/cli/e2e/container/run_test.go index 3c954ae488..488dbcec80 100644 --- a/components/cli/e2e/container/run_test.go +++ b/components/cli/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, - "docker run --rm %s echo this is output", image)) + result := icmd.RunCommand("docker", "run", "--rm", 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} -} diff --git a/components/cli/e2e/stack/remove_test.go b/components/cli/e2e/stack/remove_test.go index 0ec8639073..a75939084e 100644 --- a/components/cli/e2e/stack/remove_test.go +++ b/components/cli/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, - "docker stack deploy --compose-file=./testdata/full-stack.yml %s", stackname)) + result := icmd.RunCommand("docker", "stack", "deploy", + "--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} -} From 4485bdeb14c5cd2d4b768cf35116d94043364a72 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 23 Apr 2018 14:14:45 +0200 Subject: [PATCH 2/3] Remote FIXME(s) in vendor.conf It's coming from dependencies, doesn't make sense here Signed-off-by: Vincent Demeester Upstream-commit: 58173b3c69ed8dcc4c7926f85b44d543238bfe16 Component: cli --- components/cli/vendor.conf | 2 -- 1 file changed, 2 deletions(-) diff --git a/components/cli/vendor.conf b/components/cli/vendor.conf index e6811571d2..e34dc29fb3 100755 --- a/components/cli/vendor.conf +++ b/components/cli/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 From 637cb9eba9516b3323534c0d58000263ee1512b3 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 23 Apr 2018 14:57:29 +0200 Subject: [PATCH 3/3] Remove more TODOs - Some of them don't make sense anymore - Some are deprecated and removed from the engine since a few versions already. Signed-off-by: Vincent Demeester Upstream-commit: 740d260cd22dc144f905a33eb4db5da04fbbd8bb Component: cli --- components/cli/cli/command/container/run.go | 7 ++----- components/cli/cli/command/plugin/install.go | 5 ++--- components/cli/cli/command/plugin/remove.go | 1 - components/cli/cli/command/system/info.go | 15 --------------- 4 files changed, 4 insertions(+), 24 deletions(-) diff --git a/components/cli/cli/command/container/run.go b/components/cli/cli/command/container/run.go index 10b7ab8d9a..5458f58f1a 100644 --- a/components/cli/cli/command/container/run.go +++ b/components/cli/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 diff --git a/components/cli/cli/command/plugin/install.go b/components/cli/cli/command/plugin/install.go index 40be773bc3..88fecbfda0 100644 --- a/components/cli/cli/command/plugin/install.go +++ b/components/cli/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, - Args: opts.args, + PrivilegeFunc: registryAuthFunc, + Args: opts.args, } return options, nil } diff --git a/components/cli/cli/command/plugin/remove.go b/components/cli/cli/command/plugin/remove.go index 3513004211..e701baa850 100644 --- a/components/cli/cli/command/plugin/remove.go +++ b/components/cli/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 diff --git a/components/cli/cli/command/system/info.go b/components/cli/cli/command/system/info.go index 022bd881a0..5e6171db7d 100644 --- a/components/cli/cli/command/system/info.go +++ b/components/cli/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)