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 5fa928f50b..b9180d4e02 100644 --- a/components/cli/Makefile +++ b/components/cli/Makefile @@ -1,49 +1,51 @@ # # 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/) + ./scripts/test/unit $(shell go list ./... | grep -v /vendor/) + +.PHONY: test-coverage +test-coverage: + ./scripts/test/unit-with-coverage .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/circle.yml b/components/cli/circle.yml index e791bb93bf..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 + 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/cli/command/container/attach.go b/components/cli/cli/command/container/attach.go index 07df1dea55..e8abb1c748 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 diff --git a/components/cli/cli/command/service/opts.go b/components/cli/cli/command/service/opts.go index 96ffdd0065..c173f4f70b 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 { @@ -445,10 +395,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 } @@ -530,7 +480,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 419213417d..85ac89aa52 100644 --- a/components/cli/cli/command/service/update.go +++ b/components/cli/cli/command/service/update.go @@ -256,7 +256,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 } } @@ -963,15 +963,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/docker.Makefile b/components/cli/docker.Makefile index 3117ac538e..765b3a6f96 100644 --- a/components/cli/docker.Makefile +++ b/components/cli/docker.Makefile @@ -8,64 +8,65 @@ 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) +ENVVARS = -e VERSION=$(VERSION) -e GITCOMMIT # 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 $(ENVVARS) $(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 $(ENVVARS) $(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 --rm $(ENVVARS) $(MOUNTS) $(CROSS_IMAGE_NAME) make dynbinary 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") +} 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 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 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