From e7f90b6b38bcfd198145a41a1ee13e7788fe2f5a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 9 May 2017 18:35:25 -0400 Subject: [PATCH] Reduce complexity in cli/command/container Add tests for exec and cleanup existing tests. Signed-off-by: Daniel Nephin --- cli/command/container/attach.go | 43 +++-- cli/command/container/attach_test.go | 44 ++++- cli/command/container/client_test.go | 28 +++- cli/command/container/create.go | 36 ++--- cli/command/container/create_test.go | 64 ++++++++ cli/command/container/exec.go | 91 +++++------ cli/command/container/exec_test.go | 233 ++++++++++++++++++--------- cli/command/container/utils.go | 15 -- cli/command/service/client_test.go | 8 + cli/command/service/ps.go | 11 +- cli/command/service/ps_test.go | 22 +++ cli/compose/loader/loader.go | 1 - cli/compose/loader/volume_test.go | 10 ++ 13 files changed, 420 insertions(+), 186 deletions(-) create mode 100644 cli/command/container/create_test.go diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index dce6432846..372fcbb348 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -120,18 +120,7 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { } if c.Config.Tty && dockerCli.Out().IsTerminal() { - height, width := dockerCli.Out().GetTtySize() - // To handle the case where a user repeatedly attaches/detaches without resizing their - // terminal, the only way to get the shell prompt to display for attaches 2+ is to artificially - // resize it, then go back to normal. Without this, every attach after the first will - // require the user to manually resize or hit enter. - resizeTtyTo(ctx, client, opts.container, height+1, width+1, false) - - // After the above resizing occurs, the call to MonitorTtySize below will handle resetting back - // to the actual size. - if err := MonitorTtySize(ctx, dockerCli, opts.container, false); err != nil { - logrus.Debugf("Error monitoring TTY size: %s", err) - } + resizeTTY(ctx, dockerCli, opts.container) } streamer := hijackedIOStreamer{ @@ -151,14 +140,36 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { if errAttach != nil { return errAttach } + return getExitStatus(ctx, dockerCli.Client(), opts.container) +} - _, status, err := getExitCode(ctx, dockerCli, opts.container) - if err != nil { - return err +func resizeTTY(ctx context.Context, dockerCli command.Cli, containerID string) { + height, width := dockerCli.Out().GetTtySize() + // To handle the case where a user repeatedly attaches/detaches without resizing their + // terminal, the only way to get the shell prompt to display for attaches 2+ is to artificially + // resize it, then go back to normal. Without this, every attach after the first will + // require the user to manually resize or hit enter. + resizeTtyTo(ctx, dockerCli.Client(), containerID, height+1, width+1, false) + + // After the above resizing occurs, the call to MonitorTtySize below will handle resetting back + // to the actual size. + if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil { + logrus.Debugf("Error monitoring TTY size: %s", err) } +} + +func getExitStatus(ctx context.Context, apiclient client.ContainerAPIClient, containerID string) error { + container, err := apiclient.ContainerInspect(ctx, containerID) + if err != nil { + // If we can't connect, then the daemon probably died. + if !client.IsErrConnectionFailed(err) { + return err + } + return cli.StatusError{StatusCode: -1} + } + status := container.State.ExitCode if status != 0 { return cli.StatusError{StatusCode: status} } - return nil } diff --git a/cli/command/container/attach_test.go b/cli/command/container/attach_test.go index 8f87463af7..1ca775c6d5 100644 --- a/cli/command/container/attach_test.go +++ b/cli/command/container/attach_test.go @@ -4,10 +4,13 @@ import ( "io/ioutil" "testing" + "github.com/docker/cli/cli" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/testutil" "github.com/docker/docker/api/types" "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "golang.org/x/net/context" ) func TestNewAttachCommandErrors(t *testing.T) { @@ -67,9 +70,48 @@ func TestNewAttachCommandErrors(t *testing.T) { }, } for _, tc := range testCases { - cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{containerInspectFunc: tc.containerInspectFunc})) + cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc})) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) testutil.ErrorContains(t, cmd.Execute(), tc.expectedError) } } + +func TestGetExitStatus(t *testing.T) { + containerID := "the exec id" + expecatedErr := errors.New("unexpected error") + + testcases := []struct { + inspectError error + exitCode int + expectedError error + }{ + { + inspectError: nil, + exitCode: 0, + }, + { + inspectError: expecatedErr, + expectedError: expecatedErr, + }, + { + exitCode: 15, + expectedError: cli.StatusError{StatusCode: 15}, + }, + } + + for _, testcase := range testcases { + client := &fakeClient{ + inspectFunc: func(id string) (types.ContainerJSON, error) { + assert.Equal(t, containerID, id) + return types.ContainerJSON{ + ContainerJSONBase: &types.ContainerJSONBase{ + State: &types.ContainerState{ExitCode: testcase.exitCode}, + }, + }, testcase.inspectError + }, + } + err := getExitStatus(context.Background(), client, containerID) + assert.Equal(t, testcase.expectedError, err) + } +} diff --git a/cli/command/container/client_test.go b/cli/command/container/client_test.go index 1e2d5d9cf8..32f9a28fbf 100644 --- a/cli/command/container/client_test.go +++ b/cli/command/container/client_test.go @@ -8,12 +8,32 @@ import ( type fakeClient struct { client.Client - containerInspectFunc func(string) (types.ContainerJSON, error) + inspectFunc func(string) (types.ContainerJSON, error) + execInspectFunc func(execID string) (types.ContainerExecInspect, error) + execCreateFunc func(container string, config types.ExecConfig) (types.IDResponse, error) } -func (cli *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) { - if cli.containerInspectFunc != nil { - return cli.containerInspectFunc(containerID) +func (f *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) { + if f.inspectFunc != nil { + return f.inspectFunc(containerID) } return types.ContainerJSON{}, nil } + +func (f *fakeClient) ContainerExecCreate(_ context.Context, container string, config types.ExecConfig) (types.IDResponse, error) { + if f.execCreateFunc != nil { + return f.execCreateFunc(container, config) + } + return types.IDResponse{}, nil +} + +func (f *fakeClient) ContainerExecInspect(_ context.Context, execID string) (types.ContainerExecInspect, error) { + if f.execInspectFunc != nil { + return f.execInspectFunc(execID) + } + return types.ContainerExecInspect{}, nil +} + +func (f *fakeClient) ContainerExecStart(ctx context.Context, execID string, config types.ExecStartCheck) error { + return nil +} diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 2795e4fed7..97bc1863f0 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -113,6 +113,9 @@ type cidFile struct { } func (cid *cidFile) Close() error { + if cid.file == nil { + return nil + } cid.file.Close() if cid.written { @@ -126,6 +129,9 @@ func (cid *cidFile) Close() error { } func (cid *cidFile) Write(id string) error { + if cid.file == nil { + return nil + } if _, err := cid.file.Write([]byte(id)); err != nil { return errors.Errorf("Failed to write the container ID to the file: %s", err) } @@ -134,6 +140,9 @@ func (cid *cidFile) Write(id string) error { } func newCIDFile(path string) (*cidFile, error) { + if path == "" { + return &cidFile{}, nil + } if _, err := os.Stat(path); err == nil { return nil, errors.Errorf("Container ID file found, make sure the other container isn't running or delete %s", path) } @@ -153,19 +162,15 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig stderr := dockerCli.Err() var ( - containerIDFile *cidFile - trustedRef reference.Canonical - namedRef reference.Named + trustedRef reference.Canonical + namedRef reference.Named ) - cidfile := hostConfig.ContainerIDFile - if cidfile != "" { - var err error - if containerIDFile, err = newCIDFile(cidfile); err != nil { - return nil, err - } - defer containerIDFile.Close() + containerIDFile, err := newCIDFile(hostConfig.ContainerIDFile) + if err != nil { + return nil, err } + defer containerIDFile.Close() ref, err := reference.ParseAnyReference(config.Image) if err != nil { @@ -207,18 +212,13 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig if retryErr != nil { return nil, retryErr } - } else { - return nil, err } + return nil, err } for _, warning := range response.Warnings { fmt.Fprintf(stderr, "WARNING: %s\n", warning) } - if containerIDFile != nil { - if err = containerIDFile.Write(response.ID); err != nil { - return nil, err - } - } - return &response, nil + err = containerIDFile.Write(response.ID) + return &response, err } diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go new file mode 100644 index 0000000000..467cdb3a6e --- /dev/null +++ b/cli/command/container/create_test.go @@ -0,0 +1,64 @@ +package container + +import ( + "os" + "testing" + + "io/ioutil" + + "github.com/docker/cli/internal/test/testutil" + "github.com/gotestyourself/gotestyourself/fs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCIDFileNoOPWithNoFilename(t *testing.T) { + file, err := newCIDFile("") + require.NoError(t, err) + assert.Equal(t, &cidFile{}, file) + + assert.NoError(t, file.Write("id")) + assert.NoError(t, file.Close()) +} + +func TestNewCIDFileWhenFileAlreadyExists(t *testing.T) { + tempfile := fs.NewFile(t, "test-cid-file") + defer tempfile.Remove() + + _, err := newCIDFile(tempfile.Path()) + testutil.ErrorContains(t, err, "Container ID file found") +} + +func TestCIDFileCloseWithNoWrite(t *testing.T) { + tempdir := fs.NewDir(t, "test-cid-file") + defer tempdir.Remove() + + path := tempdir.Join("cidfile") + file, err := newCIDFile(path) + require.NoError(t, err) + assert.Equal(t, file.path, path) + + assert.NoError(t, file.Close()) + _, err = os.Stat(path) + assert.True(t, os.IsNotExist(err)) +} + +func TestCIDFileCloseWithWrite(t *testing.T) { + tempdir := fs.NewDir(t, "test-cid-file") + defer tempdir.Remove() + + path := tempdir.Join("cidfile") + file, err := newCIDFile(path) + require.NoError(t, err) + + content := "id" + assert.NoError(t, file.Write(content)) + + actual, err := ioutil.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, content, string(actual)) + + assert.NoError(t, file.Close()) + _, err = os.Stat(path) + require.NoError(t, err) +} diff --git a/cli/command/container/exec.go b/cli/command/container/exec.go index bbcd34ae0c..feb341a75d 100644 --- a/cli/command/container/exec.go +++ b/cli/command/container/exec.go @@ -7,10 +7,12 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/opts" "github.com/docker/docker/api/types" apiclient "github.com/docker/docker/client" "github.com/docker/docker/pkg/promise" + "github.com/pkg/errors" "github.com/spf13/cobra" "golang.org/x/net/context" ) @@ -22,14 +24,13 @@ type execOptions struct { detach bool user string privileged bool - env *opts.ListOpts + env opts.ListOpts + container string + command []string } -func newExecOptions() *execOptions { - var values []string - return &execOptions{ - env: opts.NewListOptsRef(&values, opts.ValidateEnv), - } +func newExecOptions() execOptions { + return execOptions{env: opts.NewListOpts(opts.ValidateEnv)} } // NewExecCommand creates a new cobra.Command for `docker exec` @@ -41,9 +42,9 @@ func NewExecCommand(dockerCli command.Cli) *cobra.Command { Short: "Run a command in a running container", Args: cli.RequiresMinArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - container := args[0] - execCmd := args[1:] - return runExec(dockerCli, options, container, execCmd) + options.container = args[0] + options.command = args[1:] + return runExec(dockerCli, options) }, } @@ -56,27 +57,14 @@ func NewExecCommand(dockerCli command.Cli) *cobra.Command { flags.BoolVarP(&options.detach, "detach", "d", false, "Detached mode: run command in the background") flags.StringVarP(&options.user, "user", "u", "", "Username or UID (format: [:])") flags.BoolVarP(&options.privileged, "privileged", "", false, "Give extended privileges to the command") - flags.VarP(options.env, "env", "e", "Set environment variables") + flags.VarP(&options.env, "env", "e", "Set environment variables") flags.SetAnnotation("env", "version", []string{"1.25"}) return cmd } -// nolint: gocyclo -func runExec(dockerCli command.Cli, options *execOptions, container string, execCmd []string) error { - execConfig, err := parseExec(options, execCmd) - // just in case the ParseExec does not exit - if container == "" || err != nil { - return cli.StatusError{StatusCode: 1} - } - - if options.detachKeys != "" { - dockerCli.ConfigFile().DetachKeys = options.detachKeys - } - - // Send client escape keys - execConfig.DetachKeys = dockerCli.ConfigFile().DetachKeys - +func runExec(dockerCli command.Cli, options execOptions) error { + execConfig := parseExec(options, dockerCli.ConfigFile()) ctx := context.Background() client := dockerCli.Client() @@ -84,7 +72,7 @@ func runExec(dockerCli command.Cli, options *execOptions, container string, exec // otherwise if we error out we will leak execIDs on the server (and // there's no easy way to clean those up). But also in order to make "not // exist" errors take precedence we do a dummy inspect first. - if _, err := client.ContainerInspect(ctx, container); err != nil { + if _, err := client.ContainerInspect(ctx, options.container); err != nil { return err } if !execConfig.Detach { @@ -93,27 +81,27 @@ func runExec(dockerCli command.Cli, options *execOptions, container string, exec } } - response, err := client.ContainerExecCreate(ctx, container, *execConfig) + response, err := client.ContainerExecCreate(ctx, options.container, *execConfig) if err != nil { return err } execID := response.ID if execID == "" { - fmt.Fprintln(dockerCli.Out(), "exec ID empty") - return nil + return errors.New("exec ID empty") } - // Temp struct for execStart so that we don't need to transfer all the execConfig. if execConfig.Detach { execStartCheck := types.ExecStartCheck{ Detach: execConfig.Detach, Tty: execConfig.Tty, } - return client.ContainerExecStart(ctx, execID, execStartCheck) } + return interactiveExec(ctx, dockerCli, execConfig, execID) +} +func interactiveExec(ctx context.Context, dockerCli command.Cli, execConfig *types.ExecConfig, execID string) error { // Interactive exec requested. var ( out, stderr io.Writer @@ -135,6 +123,7 @@ func runExec(dockerCli command.Cli, options *execOptions, container string, exec } } + client := dockerCli.Client() resp, err := client.ContainerExecAttach(ctx, execID, *execConfig) if err != nil { return err @@ -165,42 +154,35 @@ func runExec(dockerCli command.Cli, options *execOptions, container string, exec return err } - var status int - if _, status, err = getExecExitCode(ctx, client, execID); err != nil { - return err - } - - if status != 0 { - return cli.StatusError{StatusCode: status} - } - - return nil + return getExecExitStatus(ctx, client, execID) } -// getExecExitCode perform an inspect on the exec command. It returns -// the running state and the exit code. -func getExecExitCode(ctx context.Context, client apiclient.ContainerAPIClient, execID string) (bool, int, error) { +func getExecExitStatus(ctx context.Context, client apiclient.ContainerAPIClient, execID string) error { resp, err := client.ContainerExecInspect(ctx, execID) if err != nil { // If we can't connect, then the daemon probably died. if !apiclient.IsErrConnectionFailed(err) { - return false, -1, err + return err } - return false, -1, nil + return cli.StatusError{StatusCode: -1} } - - return resp.Running, resp.ExitCode, nil + status := resp.ExitCode + if status != 0 { + return cli.StatusError{StatusCode: status} + } + return nil } // parseExec parses the specified args for the specified command and generates // an ExecConfig from it. -func parseExec(opts *execOptions, execCmd []string) (*types.ExecConfig, error) { +func parseExec(opts execOptions, configFile *configfile.ConfigFile) *types.ExecConfig { execConfig := &types.ExecConfig{ User: opts.user, Privileged: opts.privileged, Tty: opts.tty, - Cmd: execCmd, + Cmd: opts.command, Detach: opts.detach, + Env: opts.env.GetAll(), } // If -d is not set, attach to everything by default @@ -212,9 +194,10 @@ func parseExec(opts *execOptions, execCmd []string) (*types.ExecConfig, error) { } } - if opts.env != nil { - execConfig.Env = opts.env.GetAll() + if opts.detachKeys != "" { + execConfig.DetachKeys = opts.detachKeys + } else { + execConfig.DetachKeys = configFile.DetachKeys } - - return execConfig, nil + return execConfig } diff --git a/cli/command/container/exec_test.go b/cli/command/container/exec_test.go index bf4fd3f35c..492e3a558d 100644 --- a/cli/command/container/exec_test.go +++ b/cli/command/container/exec_test.go @@ -4,118 +4,201 @@ import ( "io/ioutil" "testing" + "github.com/docker/cli/cli" + "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/testutil" + "github.com/docker/cli/opts" "github.com/docker/docker/api/types" "github.com/pkg/errors" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" + "golang.org/x/net/context" ) -type arguments struct { - options execOptions - execCmd []string +func withDefaultOpts(options execOptions) execOptions { + options.env = opts.NewListOpts(opts.ValidateEnv) + if len(options.command) == 0 { + options.command = []string{"command"} + } + return options } func TestParseExec(t *testing.T) { - valids := map[*arguments]*types.ExecConfig{ + testcases := []struct { + options execOptions + configFile configfile.ConfigFile + expected types.ExecConfig + }{ { - execCmd: []string{"command"}, - }: { - Cmd: []string{"command"}, - AttachStdout: true, - AttachStderr: true, + expected: types.ExecConfig{ + Cmd: []string{"command"}, + AttachStdout: true, + AttachStderr: true, + }, + options: withDefaultOpts(execOptions{}), }, { - execCmd: []string{"command1", "command2"}, - }: { - Cmd: []string{"command1", "command2"}, - AttachStdout: true, - AttachStderr: true, + expected: types.ExecConfig{ + Cmd: []string{"command1", "command2"}, + AttachStdout: true, + AttachStderr: true, + }, + options: withDefaultOpts(execOptions{ + command: []string{"command1", "command2"}, + }), }, { - options: execOptions{ + options: withDefaultOpts(execOptions{ interactive: true, tty: true, user: "uid", + }), + expected: types.ExecConfig{ + User: "uid", + AttachStdin: true, + AttachStdout: true, + AttachStderr: true, + Tty: true, + Cmd: []string{"command"}, }, - execCmd: []string{"command"}, - }: { - User: "uid", - AttachStdin: true, - AttachStdout: true, - AttachStderr: true, - Tty: true, - Cmd: []string{"command"}, }, { - options: execOptions{ - detach: true, + options: withDefaultOpts(execOptions{detach: true}), + expected: types.ExecConfig{ + Detach: true, + Cmd: []string{"command"}, }, - execCmd: []string{"command"}, - }: { - AttachStdin: false, - AttachStdout: false, - AttachStderr: false, - Detach: true, - Cmd: []string{"command"}, }, { - options: execOptions{ + options: withDefaultOpts(execOptions{ tty: true, interactive: true, detach: true, + }), + expected: types.ExecConfig{ + Detach: true, + Tty: true, + Cmd: []string{"command"}, + }, + }, + { + options: withDefaultOpts(execOptions{detach: true}), + configFile: configfile.ConfigFile{DetachKeys: "de"}, + expected: types.ExecConfig{ + Cmd: []string{"command"}, + DetachKeys: "de", + Detach: true, + }, + }, + { + options: withDefaultOpts(execOptions{ + detach: true, + detachKeys: "ab", + }), + configFile: configfile.ConfigFile{DetachKeys: "de"}, + expected: types.ExecConfig{ + Cmd: []string{"command"}, + DetachKeys: "ab", + Detach: true, }, - execCmd: []string{"command"}, - }: { - AttachStdin: false, - AttachStdout: false, - AttachStderr: false, - Detach: true, - Tty: true, - Cmd: []string{"command"}, }, } - for valid, expectedExecConfig := range valids { - execConfig, err := parseExec(&valid.options, valid.execCmd) - require.NoError(t, err) - if !compareExecConfig(expectedExecConfig, execConfig) { - t.Fatalf("Expected [%v] for %v, got [%v]", expectedExecConfig, valid, execConfig) - } + for _, testcase := range testcases { + execConfig := parseExec(testcase.options, &testcase.configFile) + assert.Equal(t, testcase.expected, *execConfig) } } -func compareExecConfig(config1 *types.ExecConfig, config2 *types.ExecConfig) bool { - if config1.AttachStderr != config2.AttachStderr { - return false +func TestRunExec(t *testing.T) { + var testcases = []struct { + doc string + options execOptions + client fakeClient + expectedError string + expectedOut string + expectedErr string + }{ + { + doc: "successful detach", + options: withDefaultOpts(execOptions{ + container: "thecontainer", + detach: true, + }), + client: fakeClient{execCreateFunc: execCreateWithID}, + }, + { + doc: "inspect error", + options: newExecOptions(), + client: fakeClient{ + inspectFunc: func(string) (types.ContainerJSON, error) { + return types.ContainerJSON{}, errors.New("failed inspect") + }, + }, + expectedError: "failed inspect", + }, + { + doc: "missing exec ID", + options: newExecOptions(), + expectedError: "exec ID empty", + }, } - if config1.AttachStdin != config2.AttachStdin { - return false + + for _, testcase := range testcases { + t.Run(testcase.doc, func(t *testing.T) { + cli := test.NewFakeCli(&testcase.client) + + err := runExec(cli, testcase.options) + if testcase.expectedError != "" { + testutil.ErrorContains(t, err, testcase.expectedError) + } else { + if !assert.NoError(t, err) { + return + } + } + assert.Equal(t, testcase.expectedOut, cli.OutBuffer().String()) + assert.Equal(t, testcase.expectedErr, cli.ErrBuffer().String()) + }) } - if config1.AttachStdout != config2.AttachStdout { - return false +} + +func execCreateWithID(_ string, _ types.ExecConfig) (types.IDResponse, error) { + return types.IDResponse{ID: "execid"}, nil +} + +func TestGetExecExitStatus(t *testing.T) { + execID := "the exec id" + expecatedErr := errors.New("unexpected error") + + testcases := []struct { + inspectError error + exitCode int + expectedError error + }{ + { + inspectError: nil, + exitCode: 0, + }, + { + inspectError: expecatedErr, + expectedError: expecatedErr, + }, + { + exitCode: 15, + expectedError: cli.StatusError{StatusCode: 15}, + }, } - if config1.Detach != config2.Detach { - return false - } - if config1.Privileged != config2.Privileged { - return false - } - if config1.Tty != config2.Tty { - return false - } - if config1.User != config2.User { - return false - } - if len(config1.Cmd) != len(config2.Cmd) { - return false - } - for index, value := range config1.Cmd { - if value != config2.Cmd[index] { - return false + + for _, testcase := range testcases { + client := &fakeClient{ + execInspectFunc: func(id string) (types.ContainerExecInspect, error) { + assert.Equal(t, execID, id) + return types.ContainerExecInspect{ExitCode: testcase.exitCode}, testcase.inspectError + }, } + err := getExecExitStatus(context.Background(), client, execID) + assert.Equal(t, testcase.expectedError, err) } - return true } func TestNewExecCommandErrors(t *testing.T) { @@ -135,7 +218,7 @@ func TestNewExecCommandErrors(t *testing.T) { }, } for _, tc := range testCases { - cli := test.NewFakeCli(&fakeClient{containerInspectFunc: tc.containerInspectFunc}) + cli := test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}) cmd := NewExecCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) diff --git a/cli/command/container/utils.go b/cli/command/container/utils.go index d9afe24163..1109b66b21 100644 --- a/cli/command/container/utils.go +++ b/cli/command/container/utils.go @@ -10,7 +10,6 @@ import ( "github.com/docker/docker/api/types/events" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/versions" - clientapi "github.com/docker/docker/client" "golang.org/x/net/context" ) @@ -125,20 +124,6 @@ func legacyWaitExitOrRemoved(ctx context.Context, dockerCli *command.DockerCli, return statusChan } -// getExitCode performs an inspect on the container. It returns -// the running state and the exit code. -func getExitCode(ctx context.Context, dockerCli command.Cli, containerID string) (bool, int, error) { - c, err := dockerCli.Client().ContainerInspect(ctx, containerID) - if err != nil { - // If we can't connect, then the daemon probably died. - if !clientapi.IsErrConnectionFailed(err) { - return false, -1, err - } - return false, -1, nil - } - return c.State.Running, c.State.ExitCode, nil -} - func parallelOperation(ctx context.Context, containers []string, op func(ctx context.Context, container string) error) chan error { if len(containers) == 0 { return nil diff --git a/cli/command/service/client_test.go b/cli/command/service/client_test.go index ebdfe7ea74..69c76951f7 100644 --- a/cli/command/service/client_test.go +++ b/cli/command/service/client_test.go @@ -14,6 +14,7 @@ type fakeClient struct { serviceInspectWithRawFunc func(ctx context.Context, serviceID string, options types.ServiceInspectOptions) (swarm.Service, []byte, error) serviceUpdateFunc func(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (types.ServiceUpdateResponse, error) serviceListFunc func(context.Context, types.ServiceListOptions) ([]swarm.Service, error) + infoFunc func(ctx context.Context) (types.Info, error) } func (f *fakeClient) NodeList(ctx context.Context, options types.NodeListOptions) ([]swarm.Node, error) { @@ -48,6 +49,13 @@ func (f *fakeClient) ServiceUpdate(ctx context.Context, serviceID string, versio return types.ServiceUpdateResponse{}, nil } +func (f *fakeClient) Info(ctx context.Context) (types.Info, error) { + if f.infoFunc == nil { + return types.Info{}, nil + } + return f.infoFunc(ctx) +} + func newService(id string, name string) swarm.Service { return swarm.Service{ ID: id, diff --git a/cli/command/service/ps.go b/cli/command/service/ps.go index 07dbba7230..e441f9e1aa 100644 --- a/cli/command/service/ps.go +++ b/cli/command/service/ps.go @@ -56,6 +56,9 @@ func runPS(dockerCli command.Cli, options psOptions) error { if err != nil { return err } + if err := updateNodeFilter(ctx, client, filter); err != nil { + return err + } tasks, err := client.TaskList(ctx, types.TaskListOptions{Filters: filter}) if err != nil { @@ -130,16 +133,20 @@ loop: if serviceCount == 0 { return filter, nil, errors.New(strings.Join(notfound, "\n")) } + return filter, notfound, err +} + +func updateNodeFilter(ctx context.Context, client client.APIClient, filter filters.Args) error { if filter.Include("node") { nodeFilters := filter.Get("node") for _, nodeFilter := range nodeFilters { nodeReference, err := node.Reference(ctx, client, nodeFilter) if err != nil { - return filter, nil, err + return err } filter.Del("node", nodeFilter) filter.Add("node", nodeReference) } } - return filter, notfound, err + return nil } diff --git a/cli/command/service/ps_test.go b/cli/command/service/ps_test.go index 1913202d33..be26870284 100644 --- a/cli/command/service/ps_test.go +++ b/cli/command/service/ps_test.go @@ -89,3 +89,25 @@ func TestRunPSWarnsOnNotFound(t *testing.T) { err := runPS(cli, options) assert.EqualError(t, err, "no such service: bar") } + +func TestUpdateNodeFilter(t *testing.T) { + selfNodeID := "foofoo" + filter := filters.NewArgs() + filter.Add("node", "one") + filter.Add("node", "two") + filter.Add("node", "self") + + client := &fakeClient{ + infoFunc: func(_ context.Context) (types.Info, error) { + return types.Info{Swarm: swarm.Info{NodeID: selfNodeID}}, nil + }, + } + + updateNodeFilter(context.Background(), client, filter) + + expected := filters.NewArgs() + expected.Add("node", "one") + expected.Add("node", "two") + expected.Add("node", selfNodeID) + assert.Equal(t, expected, filter) +} diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 2fb2630087..450603e7dd 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -576,7 +576,6 @@ func transformServiceVolumeConfig(data interface{}) (interface{}, error) { default: return data, errors.Errorf("invalid type %T for service volume", value) } - } func transformServiceNetworkMap(value interface{}) (interface{}, error) { diff --git a/cli/compose/loader/volume_test.go b/cli/compose/loader/volume_test.go index a116683327..2f2e50a905 100644 --- a/cli/compose/loader/volume_test.go +++ b/cli/compose/loader/volume_test.go @@ -200,3 +200,13 @@ func TestParseVolumeSplitCases(t *testing.T) { assert.Equal(t, expected, parsed.Source != "", msg) } } + +func TestParseVolumeInvalidEmptySpec(t *testing.T) { + _, err := ParseVolume("") + testutil.ErrorContains(t, err, "invalid empty volume spec") +} + +func TestParseVolumeInvalidSections(t *testing.T) { + _, err := ParseVolume("/foo::rw") + testutil.ErrorContains(t, err, "invalid spec") +}