From e8cc2cf7604aaa8c0f318292bc3be7eb5f60c16d Mon Sep 17 00:00:00 2001 From: Shukui Yang Date: Thu, 8 Jun 2017 07:03:52 +0800 Subject: [PATCH 1/2] Replace command.DockerCli to command.Cli in docker attach/exec command Signed-off-by: Shukui Yang --- cli/command/container/attach.go | 5 ++- cli/command/container/attach_test.go | 62 ++++++++++++++++++++++++++++ cli/command/container/client_test.go | 19 +++++++++ cli/command/container/exec.go | 4 +- cli/command/container/exec_test.go | 34 +++++++++++++++ cli/command/container/tty.go | 4 +- cli/command/container/utils.go | 2 +- 7 files changed, 123 insertions(+), 7 deletions(-) create mode 100644 cli/command/container/attach_test.go create mode 100644 cli/command/container/client_test.go diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index e8abb1c748..d9aa3db8c0 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -34,11 +34,12 @@ func inspectContainerAndCheckState(ctx context.Context, cli client.APIClient, ar if c.State.Paused { return nil, errors.New("You cannot attach to a paused container, unpause it first") } + return &c, nil } // NewAttachCommand creates a new cobra.Command for `docker attach` -func NewAttachCommand(dockerCli *command.DockerCli) *cobra.Command { +func NewAttachCommand(dockerCli command.Cli) *cobra.Command { var opts attachOptions cmd := &cobra.Command{ @@ -58,7 +59,7 @@ func NewAttachCommand(dockerCli *command.DockerCli) *cobra.Command { return cmd } -func runAttach(dockerCli *command.DockerCli, opts *attachOptions) error { +func runAttach(dockerCli command.Cli, opts *attachOptions) error { ctx := context.Background() client := dockerCli.Client() diff --git a/cli/command/container/attach_test.go b/cli/command/container/attach_test.go new file mode 100644 index 0000000000..8e29f3b076 --- /dev/null +++ b/cli/command/container/attach_test.go @@ -0,0 +1,62 @@ +package container + +import ( + "bytes" + "io/ioutil" + "testing" + + "github.com/docker/cli/cli/internal/test" + "github.com/docker/docker/api/types" + "github.com/docker/docker/pkg/testutil" + "github.com/pkg/errors" +) + +func TestNewAttachCommandErrors(t *testing.T) { + testCases := []struct { + name string + args []string + expectedError string + containerInspectFunc func(img string) (types.ContainerJSON, error) + }{ + { + name: "client-error", + args: []string{"5cb5bb5e4a3b"}, + expectedError: "something went wrong", + containerInspectFunc: func(containerID string) (types.ContainerJSON, error) { + return types.ContainerJSON{}, errors.Errorf("something went wrong") + }, + }, + { + name: "client-stopped", + args: []string{"5cb5bb5e4a3b"}, + expectedError: "You cannot attach to a stopped container", + containerInspectFunc: func(containerID string) (types.ContainerJSON, error) { + c := types.ContainerJSON{} + c.ContainerJSONBase = &types.ContainerJSONBase{} + c.ContainerJSONBase.State = &types.ContainerState{Running: false} + return c, nil + }, + }, + { + name: "client-paused", + args: []string{"5cb5bb5e4a3b"}, + expectedError: "You cannot attach to a paused container", + containerInspectFunc: func(containerID string) (types.ContainerJSON, error) { + c := types.ContainerJSON{} + c.ContainerJSONBase = &types.ContainerJSONBase{} + c.ContainerJSONBase.State = &types.ContainerState{ + Running: true, + Paused: true, + } + return c, nil + }, + }, + } + for _, tc := range testCases { + buf := new(bytes.Buffer) + cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{containerInspectFunc: tc.containerInspectFunc}, buf)) + cmd.SetOutput(ioutil.Discard) + cmd.SetArgs(tc.args) + testutil.ErrorContains(t, cmd.Execute(), tc.expectedError) + } +} diff --git a/cli/command/container/client_test.go b/cli/command/container/client_test.go new file mode 100644 index 0000000000..1e2d5d9cf8 --- /dev/null +++ b/cli/command/container/client_test.go @@ -0,0 +1,19 @@ +package container + +import ( + "github.com/docker/docker/api/types" + "github.com/docker/docker/client" + "golang.org/x/net/context" +) + +type fakeClient struct { + client.Client + containerInspectFunc func(string) (types.ContainerJSON, error) +} + +func (cli *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) { + if cli.containerInspectFunc != nil { + return cli.containerInspectFunc(containerID) + } + return types.ContainerJSON{}, nil +} diff --git a/cli/command/container/exec.go b/cli/command/container/exec.go index 587cc00f2f..843b609a0e 100644 --- a/cli/command/container/exec.go +++ b/cli/command/container/exec.go @@ -33,7 +33,7 @@ func newExecOptions() *execOptions { } // NewExecCommand creates a new cobra.Command for `docker exec` -func NewExecCommand(dockerCli *command.DockerCli) *cobra.Command { +func NewExecCommand(dockerCli command.Cli) *cobra.Command { options := newExecOptions() cmd := &cobra.Command{ @@ -63,7 +63,7 @@ func NewExecCommand(dockerCli *command.DockerCli) *cobra.Command { } // nolint: gocyclo -func runExec(dockerCli *command.DockerCli, options *execOptions, container string, execCmd []string) error { +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 { diff --git a/cli/command/container/exec_test.go b/cli/command/container/exec_test.go index e94beb4545..b2df1c2b0c 100644 --- a/cli/command/container/exec_test.go +++ b/cli/command/container/exec_test.go @@ -1,9 +1,15 @@ package container import ( + "bytes" + "io/ioutil" "testing" + "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/cli/internal/test" "github.com/docker/docker/api/types" + "github.com/docker/docker/pkg/testutil" + "github.com/pkg/errors" ) type arguments struct { @@ -114,3 +120,31 @@ func compareExecConfig(config1 *types.ExecConfig, config2 *types.ExecConfig) boo } return true } + +func TestNewExecCommandErrors(t *testing.T) { + testCases := []struct { + name string + args []string + expectedError string + containerInspectFunc func(img string) (types.ContainerJSON, error) + }{ + { + name: "client-error", + args: []string{"5cb5bb5e4a3b", "-t", "-i", "bash"}, + expectedError: "something went wrong", + containerInspectFunc: func(containerID string) (types.ContainerJSON, error) { + return types.ContainerJSON{}, errors.Errorf("something went wrong") + }, + }, + } + for _, tc := range testCases { + buf := new(bytes.Buffer) + conf := configfile.ConfigFile{} + cli := test.NewFakeCli(&fakeClient{containerInspectFunc: tc.containerInspectFunc}, buf) + cli.SetConfigfile(&conf) + cmd := NewExecCommand(cli) + cmd.SetOutput(ioutil.Discard) + cmd.SetArgs(tc.args) + testutil.ErrorContains(t, cmd.Execute(), tc.expectedError) + } +} diff --git a/cli/command/container/tty.go b/cli/command/container/tty.go index 9b09d4a0f9..fe123eb108 100644 --- a/cli/command/container/tty.go +++ b/cli/command/container/tty.go @@ -39,7 +39,7 @@ func resizeTtyTo(ctx context.Context, client client.ContainerAPIClient, id strin } // MonitorTtySize updates the container tty size when the terminal tty changes size -func MonitorTtySize(ctx context.Context, cli *command.DockerCli, id string, isExec bool) error { +func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool) error { resizeTty := func() { height, width := cli.Out().GetTtySize() resizeTtyTo(ctx, cli.Client(), id, height, width, isExec) @@ -74,7 +74,7 @@ func MonitorTtySize(ctx context.Context, cli *command.DockerCli, id string, isEx } // ForwardAllSignals forwards signals to the container -func ForwardAllSignals(ctx context.Context, cli *command.DockerCli, cid string) chan os.Signal { +func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string) chan os.Signal { sigc := make(chan os.Signal, 128) signal.CatchAll(sigc) go func() { diff --git a/cli/command/container/utils.go b/cli/command/container/utils.go index fbb38e8557..d9afe24163 100644 --- a/cli/command/container/utils.go +++ b/cli/command/container/utils.go @@ -127,7 +127,7 @@ func legacyWaitExitOrRemoved(ctx context.Context, dockerCli *command.DockerCli, // getExitCode performs an inspect on the container. It returns // the running state and the exit code. -func getExitCode(ctx context.Context, dockerCli *command.DockerCli, containerID string) (bool, int, error) { +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. From 90f497302fa3b12aa1db19b3f538262714a5d2ac Mon Sep 17 00:00:00 2001 From: Shukui Yang Date: Thu, 8 Jun 2017 07:12:39 +0800 Subject: [PATCH 2/2] Add a restarting check to runAttach Signed-off-by: Shukui Yang --- cli/command/container/attach.go | 3 +++ cli/command/container/attach_test.go | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index d9aa3db8c0..dce6432846 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -34,6 +34,9 @@ func inspectContainerAndCheckState(ctx context.Context, cli client.APIClient, ar if c.State.Paused { return nil, errors.New("You cannot attach to a paused container, unpause it first") } + if c.State.Restarting { + return nil, errors.New("You cannot attach to a restarting container, wait until it is running") + } return &c, nil } diff --git a/cli/command/container/attach_test.go b/cli/command/container/attach_test.go index 8e29f3b076..14b8137dae 100644 --- a/cli/command/container/attach_test.go +++ b/cli/command/container/attach_test.go @@ -51,6 +51,21 @@ func TestNewAttachCommandErrors(t *testing.T) { return c, nil }, }, + { + name: "client-restarting", + args: []string{"5cb5bb5e4a3b"}, + expectedError: "You cannot attach to a restarting container", + containerInspectFunc: func(containerID string) (types.ContainerJSON, error) { + c := types.ContainerJSON{} + c.ContainerJSONBase = &types.ContainerJSONBase{} + c.ContainerJSONBase.State = &types.ContainerState{ + Running: true, + Paused: false, + Restarting: true, + } + return c, nil + }, + }, } for _, tc := range testCases { buf := new(bytes.Buffer)