From 829eef48ba0f82edbb984dacf28c8e5c3e39116c Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 16 May 2017 17:49:40 +0200 Subject: [PATCH 1/8] Move duration opts into an opts package They have nothing to do with service and could be used on their own. Signed-off-by: Vincent Demeester Upstream-commit: b5182ba17ffcfdd7794775bb787a21d9c02a45ef Component: cli --- components/cli/cli/command/service/opts.go | 62 ++---------------- .../cli/cli/command/service/opts_test.go | 27 +------- components/cli/cli/command/service/update.go | 8 +-- components/cli/opts/duration.go | 64 +++++++++++++++++++ components/cli/opts/duration_test.go | 29 +++++++++ 5 files changed, 106 insertions(+), 84 deletions(-) create mode 100644 components/cli/opts/duration.go create mode 100644 components/cli/opts/duration_test.go diff --git a/components/cli/cli/command/service/opts.go b/components/cli/cli/command/service/opts.go index e1d328c487..ae1789515d 100644 --- a/components/cli/cli/command/service/opts.go +++ b/components/cli/cli/command/service/opts.go @@ -25,56 +25,6 @@ type int64Value interface { Value() int64 } -// PositiveDurationOpt is an option type for time.Duration that uses a pointer. -// It bahave similarly to DurationOpt but only allows positive duration values. -type PositiveDurationOpt struct { - DurationOpt -} - -// Set a new value on the option. Setting a negative duration value will cause -// an error to be returned. -func (d *PositiveDurationOpt) Set(s string) error { - err := d.DurationOpt.Set(s) - if err != nil { - return err - } - if *d.DurationOpt.value < 0 { - return errors.Errorf("duration cannot be negative") - } - return nil -} - -// DurationOpt is an option type for time.Duration that uses a pointer. This -// allows us to get nil values outside, instead of defaulting to 0 -type DurationOpt struct { - value *time.Duration -} - -// Set a new value on the option -func (d *DurationOpt) Set(s string) error { - v, err := time.ParseDuration(s) - d.value = &v - return err -} - -// Type returns the type of this option, which will be displayed in `--help` output -func (d *DurationOpt) Type() string { - return "duration" -} - -// String returns a string repr of this option -func (d *DurationOpt) String() string { - if d.value != nil { - return d.value.String() - } - return "" -} - -// Value returns the time.Duration -func (d *DurationOpt) Value() *time.Duration { - return d.value -} - // Uint64Opt represents a uint64. type Uint64Opt struct { value *uint64 @@ -293,9 +243,9 @@ func (r *resourceOptions) ToResourceRequirements() *swarm.ResourceRequirements { type restartPolicyOptions struct { condition string - delay DurationOpt + delay opts.DurationOpt maxAttempts Uint64Opt - window DurationOpt + window opts.DurationOpt } func defaultRestartPolicy() *swarm.RestartPolicy { @@ -444,10 +394,10 @@ func (ldo *logDriverOptions) toLogDriver() *swarm.Driver { type healthCheckOptions struct { cmd string - interval PositiveDurationOpt - timeout PositiveDurationOpt + interval opts.PositiveDurationOpt + timeout opts.PositiveDurationOpt retries int - startPeriod PositiveDurationOpt + startPeriod opts.PositiveDurationOpt noHealthcheck bool } @@ -529,7 +479,7 @@ type serviceOptions struct { hosts opts.ListOpts resources resourceOptions - stopGrace DurationOpt + stopGrace opts.DurationOpt replicas Uint64Opt mode string diff --git a/components/cli/cli/command/service/opts_test.go b/components/cli/cli/command/service/opts_test.go index 8d814f75c8..6bf00ef6bb 100644 --- a/components/cli/cli/command/service/opts_test.go +++ b/components/cli/cli/command/service/opts_test.go @@ -31,27 +31,6 @@ func TestNanoCPUsSetAndValue(t *testing.T) { assert.Equal(t, int64(350000000), cpus.Value()) } -func TestDurationOptString(t *testing.T) { - dur := time.Duration(300 * 10e8) - duration := DurationOpt{value: &dur} - assert.Equal(t, "5m0s", duration.String()) -} - -func TestDurationOptSetAndValue(t *testing.T) { - var duration DurationOpt - assert.NoError(t, duration.Set("300s")) - assert.Equal(t, time.Duration(300*10e8), *duration.Value()) - assert.NoError(t, duration.Set("-300s")) - assert.Equal(t, time.Duration(-300*10e8), *duration.Value()) -} - -func TestPositiveDurationOptSetAndValue(t *testing.T) { - var duration PositiveDurationOpt - assert.NoError(t, duration.Set("300s")) - assert.Equal(t, time.Duration(300*10e8), *duration.Value()) - assert.EqualError(t, duration.Set("-300s"), "duration cannot be negative") -} - func TestUint64OptString(t *testing.T) { value := uint64(2345678) opt := Uint64Opt{value: &value} @@ -71,9 +50,9 @@ func TestHealthCheckOptionsToHealthConfig(t *testing.T) { dur := time.Second opt := healthCheckOptions{ cmd: "curl", - interval: PositiveDurationOpt{DurationOpt{value: &dur}}, - timeout: PositiveDurationOpt{DurationOpt{value: &dur}}, - startPeriod: PositiveDurationOpt{DurationOpt{value: &dur}}, + interval: opts.PositiveDurationOpt{*opts.NewDurationOpt(&dur)}, + timeout: opts.PositiveDurationOpt{*opts.NewDurationOpt(&dur)}, + startPeriod: opts.PositiveDurationOpt{*opts.NewDurationOpt(&dur)}, retries: 10, } config, err := opt.toHealthConfig() diff --git a/components/cli/cli/command/service/update.go b/components/cli/cli/command/service/update.go index ef5452fd3c..0894da94f7 100644 --- a/components/cli/cli/command/service/update.go +++ b/components/cli/cli/command/service/update.go @@ -253,7 +253,7 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags updateDurationOpt := func(flag string, field **time.Duration) { if flags.Changed(flag) { - val := *flags.Lookup(flag).Value.(*DurationOpt).Value() + val := *flags.Lookup(flag).Value.(*opts.DurationOpt).Value() *field = &val } } @@ -960,15 +960,15 @@ func updateHealthcheck(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) containerSpec.Healthcheck.Test = nil } if flags.Changed(flagHealthInterval) { - val := *flags.Lookup(flagHealthInterval).Value.(*PositiveDurationOpt).Value() + val := *flags.Lookup(flagHealthInterval).Value.(*opts.PositiveDurationOpt).Value() containerSpec.Healthcheck.Interval = val } if flags.Changed(flagHealthTimeout) { - val := *flags.Lookup(flagHealthTimeout).Value.(*PositiveDurationOpt).Value() + val := *flags.Lookup(flagHealthTimeout).Value.(*opts.PositiveDurationOpt).Value() containerSpec.Healthcheck.Timeout = val } if flags.Changed(flagHealthStartPeriod) { - val := *flags.Lookup(flagHealthStartPeriod).Value.(*PositiveDurationOpt).Value() + val := *flags.Lookup(flagHealthStartPeriod).Value.(*opts.PositiveDurationOpt).Value() containerSpec.Healthcheck.StartPeriod = val } if flags.Changed(flagHealthRetries) { diff --git a/components/cli/opts/duration.go b/components/cli/opts/duration.go new file mode 100644 index 0000000000..5dc6eeaa73 --- /dev/null +++ b/components/cli/opts/duration.go @@ -0,0 +1,64 @@ +package opts + +import ( + "time" + + "github.com/pkg/errors" +) + +// PositiveDurationOpt is an option type for time.Duration that uses a pointer. +// It behave similarly to DurationOpt but only allows positive duration values. +type PositiveDurationOpt struct { + DurationOpt +} + +// Set a new value on the option. Setting a negative duration value will cause +// an error to be returned. +func (d *PositiveDurationOpt) Set(s string) error { + err := d.DurationOpt.Set(s) + if err != nil { + return err + } + if *d.DurationOpt.value < 0 { + return errors.Errorf("duration cannot be negative") + } + return nil +} + +// DurationOpt is an option type for time.Duration that uses a pointer. This +// allows us to get nil values outside, instead of defaulting to 0 +type DurationOpt struct { + value *time.Duration +} + +// NewDurationOpt creates a DurationOpt with the specified duration +func NewDurationOpt(value *time.Duration) *DurationOpt { + return &DurationOpt{ + value: value, + } +} + +// Set a new value on the option +func (d *DurationOpt) Set(s string) error { + v, err := time.ParseDuration(s) + d.value = &v + return err +} + +// Type returns the type of this option, which will be displayed in `--help` output +func (d *DurationOpt) Type() string { + return "duration" +} + +// String returns a string repr of this option +func (d *DurationOpt) String() string { + if d.value != nil { + return d.value.String() + } + return "" +} + +// Value returns the time.Duration +func (d *DurationOpt) Value() *time.Duration { + return d.value +} diff --git a/components/cli/opts/duration_test.go b/components/cli/opts/duration_test.go new file mode 100644 index 0000000000..f766a1c74c --- /dev/null +++ b/components/cli/opts/duration_test.go @@ -0,0 +1,29 @@ +package opts + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestDurationOptString(t *testing.T) { + dur := time.Duration(300 * 10e8) + duration := DurationOpt{value: &dur} + assert.Equal(t, "5m0s", duration.String()) +} + +func TestDurationOptSetAndValue(t *testing.T) { + var duration DurationOpt + assert.NoError(t, duration.Set("300s")) + assert.Equal(t, time.Duration(300*10e8), *duration.Value()) + assert.NoError(t, duration.Set("-300s")) + assert.Equal(t, time.Duration(-300*10e8), *duration.Value()) +} + +func TestPositiveDurationOptSetAndValue(t *testing.T) { + var duration PositiveDurationOpt + assert.NoError(t, duration.Set("300s")) + assert.Equal(t, time.Duration(300*10e8), *duration.Value()) + assert.EqualError(t, duration.Set("-300s"), "duration cannot be negative") +} From 235c74487c0700376b16991b16189e913a140cae Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 17 May 2017 12:08:50 -0400 Subject: [PATCH 2/8] Remove @ from Makefiles. Signed-off-by: Daniel Nephin Upstream-commit: 73ebb07df5faa8c7906affc1933ff1bca2a62bbd Component: cli --- components/cli/Makefile | 26 ++++++++++++-------------- components/cli/docker.Makefile | 25 ++++++++++++------------- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/components/cli/Makefile b/components/cli/Makefile index 5fa928f50b..566da5a636 100644 --- a/components/cli/Makefile +++ b/components/cli/Makefile @@ -1,49 +1,47 @@ # # github.com/docker/cli # - all: binary # remove build artifacts .PHONY: clean clean: - @rm -rf ./build/* cli/winresources/rsrc_* + rm -rf ./build/* cli/winresources/rsrc_* # run go test # the "-tags daemon" part is temporary .PHONY: test test: - @go test -tags daemon -v $(shell go list ./... | grep -v /vendor/) + go test -tags daemon -v $(shell go list ./... | grep -v /vendor/) .PHONY: lint lint: - @gometalinter --config gometalinter.json ./... + gometalinter --config gometalinter.json ./... .PHONY: binary binary: - @./scripts/build/binary + @echo "WARNING: binary creates a Linux executable. Use cross for macOS or Windows." + ./scripts/build/binary -# build the CLI for multiple architectures .PHONY: cross cross: - @./scripts/build/cross + ./scripts/build/cross .PHONY: dynbinary dynbinary: - @./scripts/build/dynbinary + ./scripts/build/dynbinary .PHONY: watch watch: - @./scripts/test/watch + ./scripts/test/watch -# download dependencies (vendor/) listed in vendor.conf -.PHONY: vendor +# Check vendor matches vendor.conf vendor: vendor.conf - @vndr 2> /dev/null - @scripts/validate/check-git-diff vendor + vndr 2> /dev/null + scripts/validate/check-git-diff vendor cli/compose/schema/bindata.go: cli/compose/schema/data/*.json go generate github.com/docker/cli/cli/compose/schema compose-jsonschema: cli/compose/schema/bindata.go - @scripts/validate/check-git-diff cli/compose/schema/bindata.go + scripts/validate/check-git-diff cli/compose/schema/bindata.go diff --git a/components/cli/docker.Makefile b/components/cli/docker.Makefile index 3117ac538e..b82d0e6e62 100644 --- a/components/cli/docker.Makefile +++ b/components/cli/docker.Makefile @@ -12,60 +12,59 @@ MOUNTS = -v `pwd`:/go/src/github.com/docker/cli # build docker image (dockerfiles/Dockerfile.build) .PHONY: build_docker_image build_docker_image: - @docker build -t $(DEV_DOCKER_IMAGE_NAME) -f ./dockerfiles/Dockerfile.dev . + docker build -t $(DEV_DOCKER_IMAGE_NAME) -f ./dockerfiles/Dockerfile.dev . # build docker image having the linting tools (dockerfiles/Dockerfile.lint) .PHONY: build_linter_image build_linter_image: - @docker build -t $(LINTER_IMAGE_NAME) -f ./dockerfiles/Dockerfile.lint . + docker build -t $(LINTER_IMAGE_NAME) -f ./dockerfiles/Dockerfile.lint . .PHONY: build_cross_image build_cross_image: - @docker build -t $(CROSS_IMAGE_NAME) -f ./dockerfiles/Dockerfile.cross . + docker build -t $(CROSS_IMAGE_NAME) -f ./dockerfiles/Dockerfile.cross . # build executable using a container binary: build_docker_image - @echo "WARNING: this will drop a Linux executable on your host (not a macOS or Windows one)" - @docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary + docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary build: binary # clean build artifacts using a container .PHONY: clean clean: build_docker_image - @docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make clean + docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make clean # run go test .PHONY: test test: build_docker_image - @docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make test + docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make test # build the CLI for multiple architectures using a container .PHONY: cross cross: build_cross_image - @docker run --rm $(MOUNTS) $(CROSS_IMAGE_NAME) make cross + docker run --rm $(MOUNTS) $(CROSS_IMAGE_NAME) make cross .PHONY: watch watch: build_docker_image - @docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make watch + docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make watch # start container in interactive mode for in-container development .PHONY: dev dev: build_docker_image - @docker run -ti $(MOUNTS) -v /var/run/docker.sock:/var/run/docker.sock $(DEV_DOCKER_IMAGE_NAME) ash + docker run -ti $(MOUNTS) -v /var/run/docker.sock:/var/run/docker.sock $(DEV_DOCKER_IMAGE_NAME) ash shell: dev # run linters in a container .PHONY: lint lint: build_linter_image - @docker run -ti $(MOUNTS) $(LINTER_IMAGE_NAME) + docker run -ti $(MOUNTS) $(LINTER_IMAGE_NAME) # download dependencies (vendor/) listed in vendor.conf, using a container .PHONY: vendor vendor: build_docker_image vendor.conf - @docker run -ti --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make vendor + docker run -ti --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make vendor dynbinary: build_cross_image - @docker run -ti --rm $(MOUNTS) $(CROSS_IMAGE_NAME) make dynbinary + docker run -ti --rm $(MOUNTS) $(CROSS_IMAGE_NAME) make dynbinary From 4d54b449191f06618ebd1c76fe34d4191caa1f2e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 17 May 2017 11:59:45 -0400 Subject: [PATCH 3/8] Enabled linux/ppc64le cross build. Signed-off-by: Daniel Nephin Upstream-commit: 46049dd0b68607fd44251b79c991f0db1b4e84ec Component: cli --- components/cli/scripts/build/linux-cross | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/components/cli/scripts/build/linux-cross b/components/cli/scripts/build/linux-cross index 4a43e4b396..53aa0e1511 100755 --- a/components/cli/scripts/build/linux-cross +++ b/components/cli/scripts/build/linux-cross @@ -7,10 +7,7 @@ set -eu -o pipefail source ./scripts/build/.variables -CROSS_OSARCH="linux/amd64 linux/arm" - -# Compile is broken -# linux/ppc64le +CROSS_OSARCH="linux/amd64 linux/arm linux/ppc64le" # Not yet supported by gox # linux/s390x From 56d05cc358da9246b2a5cafe7b87e57f830153f7 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Thu, 18 May 2017 10:48:24 +0200 Subject: [PATCH 4/8] Add test-coverage & codecov target and update circleci Signed-off-by: Vincent Demeester Upstream-commit: d79d90386428baeadf7b74dd99ceef03c38df974 Component: cli --- components/cli/.gitignore | 2 ++ components/cli/Makefile | 13 +++++++++++++ components/cli/circle.yml | 2 +- components/cli/dockerfiles/Dockerfile.dev | 2 +- 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/components/cli/.gitignore b/components/cli/.gitignore index 4a65c2279c..09d363d1ce 100644 --- a/components/cli/.gitignore +++ b/components/cli/.gitignore @@ -2,3 +2,5 @@ /build/ cli/winresources/rsrc_386.syso cli/winresources/rsrc_amd64.syso +coverage.txt +profile.out diff --git a/components/cli/Makefile b/components/cli/Makefile index 566da5a636..3320659709 100644 --- a/components/cli/Makefile +++ b/components/cli/Makefile @@ -14,6 +14,19 @@ clean: test: go test -tags daemon -v $(shell go list ./... | grep -v /vendor/) +.PHONY: test-coverage +test-coverage: + for pkg in $(shell go list ./... | grep -v /vendor/); do \ + go test -tags daemon -v -cover -parallel 8 -coverprofile=profile.out -covermode=atomic $${pkg} || exit 1; \ + if test -f profile.out; then \ + cat profile.out >> coverage.txt && rm profile.out; \ + fi; \ + done + +.PHONY: codecov +codecov: + $(shell curl -s https://codecov.io/bash | bash) + .PHONY: lint lint: gometalinter --config gometalinter.json ./... diff --git a/components/cli/circle.yml b/components/cli/circle.yml index e791bb93bf..be160473e1 100644 --- a/components/cli/circle.yml +++ b/components/cli/circle.yml @@ -35,7 +35,7 @@ jobs: dockerfile=dockerfiles/Dockerfile.dev echo "COPY . ." >> $dockerfile docker build -f $dockerfile --tag cli-builder . - docker run cli-builder make test + docker run cli-builder make test-coverage codecov - run: name: "Validate Vendor and Code Generation" command: | diff --git a/components/cli/dockerfiles/Dockerfile.dev b/components/cli/dockerfiles/Dockerfile.dev index a14ea25f29..2886ed5e1a 100644 --- a/components/cli/dockerfiles/Dockerfile.dev +++ b/components/cli/dockerfiles/Dockerfile.dev @@ -1,7 +1,7 @@ FROM golang:1.8-alpine -RUN apk add -U git make bash coreutils +RUN apk add -U git make bash coreutils curl RUN go get github.com/LK4D4/vndr && \ cp /go/bin/vndr /usr/bin && \ From f039c950a8e5ab801dca6524c6354ee9ba02a235 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 18 May 2017 10:42:05 -0400 Subject: [PATCH 5/8] Upload coverage report from outside of test container. Signed-off-by: Daniel Nephin Upstream-commit: f6d148c632bf4e672baffef9a22ccf502bbd37a0 Component: cli --- components/cli/Makefile | 13 ++----------- components/cli/circle.yml | 7 +++++-- components/cli/dockerfiles/Dockerfile.dev | 2 +- components/cli/scripts/test/unit | 4 ++++ components/cli/scripts/test/unit-with-coverage | 15 +++++++++++++++ 5 files changed, 27 insertions(+), 14 deletions(-) create mode 100755 components/cli/scripts/test/unit create mode 100755 components/cli/scripts/test/unit-with-coverage diff --git a/components/cli/Makefile b/components/cli/Makefile index 3320659709..b9180d4e02 100644 --- a/components/cli/Makefile +++ b/components/cli/Makefile @@ -12,20 +12,11 @@ clean: # the "-tags daemon" part is temporary .PHONY: test test: - go test -tags daemon -v $(shell go list ./... | grep -v /vendor/) + ./scripts/test/unit $(shell go list ./... | grep -v /vendor/) .PHONY: test-coverage test-coverage: - for pkg in $(shell go list ./... | grep -v /vendor/); do \ - go test -tags daemon -v -cover -parallel 8 -coverprofile=profile.out -covermode=atomic $${pkg} || exit 1; \ - if test -f profile.out; then \ - cat profile.out >> coverage.txt && rm profile.out; \ - fi; \ - done - -.PHONY: codecov -codecov: - $(shell curl -s https://codecov.io/bash | bash) + ./scripts/test/unit-with-coverage .PHONY: lint lint: diff --git a/components/cli/circle.yml b/components/cli/circle.yml index be160473e1..dabc4c5aad 100644 --- a/components/cli/circle.yml +++ b/components/cli/circle.yml @@ -29,13 +29,16 @@ jobs: docker run --name cross cli-builder make cross docker cp cross:/go/src/github.com/docker/cli/build /work/build - run: - name: "Unit Test" + name: "Unit Test with Coverage" command: | if [ "$CIRCLE_NODE_INDEX" != "2" ]; then exit; fi dockerfile=dockerfiles/Dockerfile.dev echo "COPY . ." >> $dockerfile docker build -f $dockerfile --tag cli-builder . - docker run cli-builder make test-coverage codecov + docker run --name test cli-builder make test-coverage + docker cp test:/go/src/github.com/docker/cli/coverage.txt coverage.txt + apk add -U bash curl + curl -s https://codecov.io/bash | bash - run: name: "Validate Vendor and Code Generation" command: | diff --git a/components/cli/dockerfiles/Dockerfile.dev b/components/cli/dockerfiles/Dockerfile.dev index 2886ed5e1a..a14ea25f29 100644 --- a/components/cli/dockerfiles/Dockerfile.dev +++ b/components/cli/dockerfiles/Dockerfile.dev @@ -1,7 +1,7 @@ FROM golang:1.8-alpine -RUN apk add -U git make bash coreutils curl +RUN apk add -U git make bash coreutils RUN go get github.com/LK4D4/vndr && \ cp /go/bin/vndr /usr/bin && \ diff --git a/components/cli/scripts/test/unit b/components/cli/scripts/test/unit new file mode 100755 index 0000000000..8c2f9c75df --- /dev/null +++ b/components/cli/scripts/test/unit @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -eu -o pipefail + +go test -tags daemon -v $@ diff --git a/components/cli/scripts/test/unit-with-coverage b/components/cli/scripts/test/unit-with-coverage new file mode 100755 index 0000000000..9073e2daf6 --- /dev/null +++ b/components/cli/scripts/test/unit-with-coverage @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +set -eu -o pipefail + +for pkg in $(go list ./... | grep -v /vendor/); do + ./scripts/test/unit \ + -cover \ + -coverprofile=profile.out \ + -covermode=atomic \ + ${pkg} + + if test -f profile.out; then + cat profile.out >> coverage.txt + rm profile.out + fi +done From 1959f587f47b193d934bcfcf0f1694e81ff74814 Mon Sep 17 00:00:00 2001 From: Shukui Yang Date: Thu, 18 May 2017 12:12:37 +0800 Subject: [PATCH 6/8] Recheck the container's state to avoid attach block. If use docker attach command to attach to a stop container, it will return "You cannot attach to a stopped container" error, it's ok, but when attach to a running container, it(docker attach) use inspect to check the container's state, if it pass the state check on the client side, and then the container is stopped, docker attach command still attach to the container and not exit. Signed-off-by: Shukui Yang Upstream-commit: f9dc3337f9dd3b02cb4fb888b6237c75ebdd644b Component: cli --- .../cli/cli/command/container/attach.go | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/components/cli/cli/command/container/attach.go b/components/cli/cli/command/container/attach.go index f86b33a8ca..39a59e3926 100644 --- a/components/cli/cli/command/container/attach.go +++ b/components/cli/cli/command/container/attach.go @@ -8,6 +8,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/docker/api/types" + "github.com/docker/docker/client" "github.com/docker/docker/pkg/signal" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -22,6 +23,20 @@ type attachOptions struct { container string } +func inspectContainerAndCheckState(ctx context.Context, cli client.APIClient, args string) (*types.ContainerJSON, error) { + c, err := cli.ContainerInspect(ctx, args) + if err != nil { + return nil, err + } + if !c.State.Running { + return nil, errors.New("You cannot attach to a stopped container, start it first") + } + 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 { var opts attachOptions @@ -47,19 +62,11 @@ func runAttach(dockerCli *command.DockerCli, opts *attachOptions) error { ctx := context.Background() client := dockerCli.Client() - c, err := client.ContainerInspect(ctx, opts.container) + c, err := inspectContainerAndCheckState(ctx, client, opts.container) if err != nil { return err } - if !c.State.Running { - return errors.New("You cannot attach to a stopped container, start it first") - } - - if c.State.Paused { - return errors.New("You cannot attach to a paused container, unpause it first") - } - if err := dockerCli.In().CheckTty(!opts.noStdin, c.Config.Tty); err != nil { return err } @@ -95,6 +102,19 @@ func runAttach(dockerCli *command.DockerCli, opts *attachOptions) error { } defer resp.Close() + // If use docker attach command to attach to a stop container, it will return + // "You cannot attach to a stopped container" error, it's ok, but when + // attach to a running container, it(docker attach) use inspect to check + // the container's state, if it pass the state check on the client side, + // and then the container is stopped, docker attach command still attach to + // the container and not exit. + // + // Recheck the container's state to avoid attach block. + _, err = inspectContainerAndCheckState(ctx, client, opts.container) + if err != nil { + return err + } + 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 From 1d9c61db3a9c8e628f00a05ffb16657a6297d819 Mon Sep 17 00:00:00 2001 From: Andrew Hsu Date: Mon, 22 May 2017 13:22:42 -0700 Subject: [PATCH 7/8] allow version number to be set in builds Signed-off-by: Andrew Hsu Upstream-commit: 3dfe334a7a203efb0c7866d6590e98b9402aa08e Component: cli --- components/cli/docker.Makefile | 5 +++-- components/cli/scripts/build/osx | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/components/cli/docker.Makefile b/components/cli/docker.Makefile index b82d0e6e62..9877124991 100644 --- a/components/cli/docker.Makefile +++ b/components/cli/docker.Makefile @@ -8,6 +8,7 @@ DEV_DOCKER_IMAGE_NAME = docker-cli-dev LINTER_IMAGE_NAME = docker-cli-lint CROSS_IMAGE_NAME = docker-cli-cross MOUNTS = -v `pwd`:/go/src/github.com/docker/cli +VERSION = $(shell cat VERSION) # build docker image (dockerfiles/Dockerfile.build) .PHONY: build_docker_image @@ -26,7 +27,7 @@ build_cross_image: # build executable using a container binary: build_docker_image - docker run --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary + docker run --rm -e VERSION=$(VERSION) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary build: binary @@ -43,7 +44,7 @@ test: build_docker_image # build the CLI for multiple architectures using a container .PHONY: cross cross: build_cross_image - docker run --rm $(MOUNTS) $(CROSS_IMAGE_NAME) make cross + docker run --rm -e VERSION=$(VERSION) $(MOUNTS) $(CROSS_IMAGE_NAME) make cross .PHONY: watch watch: build_docker_image diff --git a/components/cli/scripts/build/osx b/components/cli/scripts/build/osx index b993a8e0c8..a5e7d9432c 100755 --- a/components/cli/scripts/build/osx +++ b/components/cli/scripts/build/osx @@ -11,7 +11,7 @@ export CGO_ENABLED=1 export GOOS=darwin export GOARCH=amd64 export CC=o64-clang -export LDFLAGS='-linkmode external -s' +export LDFLAGS="$LDFLAGS -linkmode external -s" export LDFLAGS_STATIC_DOCKER='-extld='${CC} # Override TARGET From 9cdca5ef2e7d1e5b2f744a60f1b9674e4f689741 Mon Sep 17 00:00:00 2001 From: Andrew Hsu Date: Wed, 24 May 2017 17:26:53 +0000 Subject: [PATCH 8/8] pass in optional GITCOMMIT override Signed-off-by: Andrew Hsu Upstream-commit: 446af3a9b7352c524ab3fadb06ca3ad6d4ea79a2 Component: cli --- components/cli/docker.Makefile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/components/cli/docker.Makefile b/components/cli/docker.Makefile index 9877124991..765b3a6f96 100644 --- a/components/cli/docker.Makefile +++ b/components/cli/docker.Makefile @@ -9,6 +9,7 @@ LINTER_IMAGE_NAME = docker-cli-lint CROSS_IMAGE_NAME = docker-cli-cross MOUNTS = -v `pwd`:/go/src/github.com/docker/cli VERSION = $(shell cat VERSION) +ENVVARS = -e VERSION=$(VERSION) -e GITCOMMIT # build docker image (dockerfiles/Dockerfile.build) .PHONY: build_docker_image @@ -27,7 +28,7 @@ build_cross_image: # build executable using a container binary: build_docker_image - docker run --rm -e VERSION=$(VERSION) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary + docker run --rm $(ENVVARS) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary build: binary @@ -44,7 +45,7 @@ test: build_docker_image # build the CLI for multiple architectures using a container .PHONY: cross cross: build_cross_image - docker run --rm -e VERSION=$(VERSION) $(MOUNTS) $(CROSS_IMAGE_NAME) make cross + docker run --rm $(ENVVARS) $(MOUNTS) $(CROSS_IMAGE_NAME) make cross .PHONY: watch watch: build_docker_image @@ -68,4 +69,4 @@ vendor: build_docker_image vendor.conf docker run -ti --rm $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make vendor dynbinary: build_cross_image - docker run -ti --rm $(MOUNTS) $(CROSS_IMAGE_NAME) make dynbinary + docker run --rm $(ENVVARS) $(MOUNTS) $(CROSS_IMAGE_NAME) make dynbinary