Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 23 additions & 17 deletions cli/command/container/attach.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package container

import (
"fmt"
"io"
"net/http/httputil"

"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/client"
"github.com/docker/docker/pkg/signal"
"github.com/pkg/errors"
Expand Down Expand Up @@ -66,6 +68,9 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
ctx := context.Background()
client := dockerCli.Client()

// request channel to wait for client
resultC, errC := client.ContainerWait(ctx, opts.container, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually I think this should come after inspectContainerAndCheckState() which is a sanity check on the state of the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid a race when by the time we return from inspectContainerAndCheckState() the container had exited, but I don't mind moving it afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, maybe it is more correct before the inspect. The error from ContainerWait() wouldn't be reported until later, so the user would still see the helpful error message.

I think this is correct, ignore my comment.


c, err := inspectContainerAndCheckState(ctx, client, opts.container)
if err != nil {
return err
Expand Down Expand Up @@ -140,7 +145,24 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
if errAttach != nil {
return errAttach
}
return getExitStatus(ctx, dockerCli.Client(), opts.container)

return getExitStatus(errC, resultC)
}

func getExitStatus(errC <-chan error, resultC <-chan container.ContainerWaitOKBody) error {
select {
case result := <-resultC:
if result.Error != nil {
return fmt.Errorf(result.Error.Message)
}
if result.StatusCode != 0 {
return cli.StatusError{StatusCode: int(result.StatusCode)}
}
case err := <-errC:
return err
}

return nil
}

func resizeTTY(ctx context.Context, dockerCli command.Cli, containerID string) {
Expand All @@ -157,19 +179,3 @@ func resizeTTY(ctx context.Context, dockerCli command.Cli, containerID string) {
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
}
51 changes: 31 additions & 20 deletions cli/command/container/attach_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
package container

import (
"fmt"
"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/docker/docker/api/types/container"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"golang.org/x/net/context"
)

func TestNewAttachCommandErrors(t *testing.T) {
Expand Down Expand Up @@ -78,40 +79,50 @@ func TestNewAttachCommandErrors(t *testing.T) {
}

func TestGetExitStatus(t *testing.T) {
containerID := "the exec id"
expecatedErr := errors.New("unexpected error")
var (
expectedErr = fmt.Errorf("unexpected error")
errC = make(chan error, 1)
resultC = make(chan container.ContainerWaitOKBody, 1)
)

testcases := []struct {
inspectError error
exitCode int
result *container.ContainerWaitOKBody
err error
expectedError error
}{
{
inspectError: nil,
exitCode: 0,
result: &container.ContainerWaitOKBody{
StatusCode: 0,
},
},
{
err: expectedErr,
expectedError: expectedErr,
},
{
inspectError: expecatedErr,
expectedError: expecatedErr,
result: &container.ContainerWaitOKBody{
Error: &container.ContainerWaitOKBodyError{
expectedErr.Error(),
},
},
expectedError: expectedErr,
},
{
exitCode: 15,
result: &container.ContainerWaitOKBody{
StatusCode: 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
},
if testcase.err != nil {
errC <- testcase.err
}
if testcase.result != nil {
resultC <- *testcase.result
}
err := getExitStatus(context.Background(), client, containerID)
err := getExitStatus(errC, resultC)
assert.Equal(t, testcase.expectedError, err)
}
}
29 changes: 29 additions & 0 deletions e2e/container/attach_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package container

import (
"strings"
"testing"

"github.com/gotestyourself/gotestyourself/icmd"
)

func TestAttachExitCode(t *testing.T) {
containerID := runBackgroundContainsWithExitCode(t, 21)

result := icmd.RunCmd(
icmd.Command("docker", "attach", containerID),
withStdinNewline)

result.Assert(t, icmd.Expected{ExitCode: 21})
}

func runBackgroundContainsWithExitCode(t *testing.T, exitcode int) string {
result := icmd.RunCmd(shell(t,
"docker run -d -i --rm %s sh -c 'read; exit %d'", alpineImage, exitcode))
result.Assert(t, icmd.Success)
return strings.TrimSpace(result.Stdout())
}

func withStdinNewline(cmd *icmd.Cmd) {
cmd.Stdin = strings.NewReader("\n")
}