Reduce complexity in cli/command/container

Add tests for exec and cleanup existing tests.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

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()
+		resizeTTY(ctx, dockerCli, opts.container)
-		// 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)
-		}
 	}
 
 	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)
+func resizeTTY(ctx context.Context, dockerCli command.Cli, containerID string) {
-	if err != nil {
+	height, width := dockerCli.Out().GetTtySize()
-		return err
+	// 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
 }

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)
+	}
+}

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) {
+func (f *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) {
-	if cli.containerInspectFunc != nil {
+	if f.inspectFunc != nil {
-		return cli.containerInspectFunc(containerID)
+		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
+}

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
-		trustedRef      reference.Canonical
+		namedRef   reference.Named
-		namedRef        reference.Named
 	)
 
-	cidfile := hostConfig.ContainerIDFile
+	containerIDFile, err := newCIDFile(hostConfig.ContainerIDFile)
-	if cidfile != "" {
+	if err != nil {
-		var err error
+		return nil, err
-		if containerIDFile, err = newCIDFile(cidfile); err != nil {
-			return nil, err
-		}
-		defer containerIDFile.Close()
 	}
+	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 {
+	err = containerIDFile.Write(response.ID)
-		if err = containerIDFile.Write(response.ID); err != nil {
+	return &response, err
-			return nil, err
-		}
-	}
-	return &response, nil
 }

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)
+}

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 {
+func newExecOptions() execOptions {
-	var values []string
+	return execOptions{env: opts.NewListOpts(opts.ValidateEnv)}
-	return &execOptions{
-		env: opts.NewListOptsRef(&values, 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]
+			options.container = args[0]
-			execCmd := args[1:]
+			options.command = args[1:]
-			return runExec(dockerCli, options, container, execCmd)
+			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: <name|uid>[:<group|gid>])")
 	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) error {
-func runExec(dockerCli command.Cli, options *execOptions, container string, execCmd []string) error {
+	execConfig := parseExec(options, dockerCli.ConfigFile())
-	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
-
 	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 errors.New("exec ID empty")
-		return nil
 	}
 
-	// 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
+	return getExecExitStatus(ctx, client, execID)
-	if _, status, err = getExecExitCode(ctx, client, execID); err != nil {
-		return err
-	}
-
-	if status != 0 {
-		return cli.StatusError{StatusCode: status}
-	}
-
-	return nil
 }
 
-// getExecExitCode perform an inspect on the exec command. It returns
+func getExecExitStatus(ctx context.Context, client apiclient.ContainerAPIClient, execID string) error {
-// the running state and the exit code.
-func getExecExitCode(ctx context.Context, client apiclient.ContainerAPIClient, execID string) (bool, int, 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}
 	}
-
+	status := resp.ExitCode
-	return resp.Running, resp.ExitCode, nil
+	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 {
+	if opts.detachKeys != "" {
-		execConfig.Env = opts.env.GetAll()
+		execConfig.DetachKeys = opts.detachKeys
+	} else {
+		execConfig.DetachKeys = configFile.DetachKeys
 	}
-
+	return execConfig
-	return execConfig, nil
 }

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 {
+func withDefaultOpts(options execOptions) execOptions {
-	options execOptions
+	options.env = opts.NewListOpts(opts.ValidateEnv)
-	execCmd []string
+	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"},
+			expected: types.ExecConfig{
-		}: {
+				Cmd:          []string{"command"},
-			Cmd:          []string{"command"},
+				AttachStdout: true,
-			AttachStdout: true,
+				AttachStderr: true,
-			AttachStderr: true,
+			},
+			options: withDefaultOpts(execOptions{}),
 		},
 		{
-			execCmd: []string{"command1", "command2"},
+			expected: types.ExecConfig{
-		}: {
+				Cmd:          []string{"command1", "command2"},
-			Cmd:          []string{"command1", "command2"},
+				AttachStdout: true,
-			AttachStdout: true,
+				AttachStderr: 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{
+			options: withDefaultOpts(execOptions{detach: true}),
-				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 {
+	for _, testcase := range testcases {
-		execConfig, err := parseExec(&valid.options, valid.execCmd)
+		execConfig := parseExec(testcase.options, &testcase.configFile)
-		require.NoError(t, err)
+		assert.Equal(t, testcase.expected, *execConfig)
-		if !compareExecConfig(expectedExecConfig, execConfig) {
-			t.Fatalf("Expected [%v] for %v, got [%v]", expectedExecConfig, valid, execConfig)
-		}
 	}
 }
 
-func compareExecConfig(config1 *types.ExecConfig, config2 *types.ExecConfig) bool {
+func TestRunExec(t *testing.T) {
-	if config1.AttachStderr != config2.AttachStderr {
+	var testcases = []struct {
-		return false
+		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
+	for _, testcase := range testcases {
-	}
+		client := &fakeClient{
-	if config1.Privileged != config2.Privileged {
+			execInspectFunc: func(id string) (types.ContainerExecInspect, error) {
-		return false
+				assert.Equal(t, execID, id)
-	}
+				return types.ContainerExecInspect{ExitCode: testcase.exitCode}, testcase.inspectError
-	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
 		}
+		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)

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

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,

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
 }

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)
+}

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) {

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")
+}