Review comments

Address code review comemnts and purge additional dead code. Signed-off-by: Daniel Hiltgen <[email protected]>
docker · Sep 20, 2018 · f250152 · f250152
1 parent 342afe4
commit f250152
Show file tree

Hide file tree

Showing 26 changed files with 77 additions and 1,323 deletions.
diff --git a/Makefile b/Makefile
@@ -12,14 +12,14 @@ clean: ## remove build artifacts
 
 .PHONY: test-unit
 test-unit: ## run unit test
-	./scripts/test/unit $(shell go list ./... | grep -vE '/vendor/|/e2e/|/e2eengine/')
+	./scripts/test/unit $(shell go list ./... | grep -vE '/vendor/|/e2e/')
 
 .PHONY: test
 test: test-unit ## run tests
 
 .PHONY: test-coverage
 test-coverage: ## run test coverage
-	./scripts/test/unit-with-coverage $(shell go list ./... | grep -vE '/vendor/|/e2e/|/e2eengine/')
+	./scripts/test/unit-with-coverage $(shell go list ./... | grep -vE '/vendor/|/e2e/')
 
 .PHONY: lint
 lint: ## run all the lint tools

diff --git a/cli/command/engine/activate.go b/cli/command/engine/activate.go
@@ -56,7 +56,7 @@ https://hub.docker.com/ then specify the file with the '--license' flag.
 
 	flags.StringVar(&options.licenseFile, "license", "", "License File")
 	flags.StringVar(&options.version, "version", "", "Specify engine version (default is to use currently running version)")
-	flags.StringVar(&options.registryPrefix, "registry-prefix", "docker.io/store/docker", "Override the default location where engine images are pulled")
+	flags.StringVar(&options.registryPrefix, "registry-prefix", clitypes.RegistryPrefix, "Override the default location where engine images are pulled")
 	flags.StringVar(&options.image, "engine-image", clitypes.EnterpriseEngineImage, "Specify engine image")
 	flags.StringVar(&options.format, "format", "", "Pretty-print licenses using a Go template")
 	flags.BoolVar(&options.displayOnly, "display-only", false, "only display the available licenses and exit")
@@ -68,7 +68,7 @@ https://hub.docker.com/ then specify the file with the '--license' flag.
 
 func runActivate(cli command.Cli, options activateOptions) error {
 	if !isRoot() {
-		return errors.New("must be privileged to activate engine")
+		return errors.New("this command must be run as a privileged user")
 	}
 	ctx := context.Background()
 	client, err := cli.NewContainerizedEngineClient(options.sockPath)
@@ -107,16 +107,16 @@ func runActivate(cli command.Cli, options activateOptions) error {
 		EngineVersion:  options.version,
 	}
 
-	err = client.ActivateEngine(ctx, opts, cli.Out(), authConfig,
+	if err := client.ActivateEngine(ctx, opts, cli.Out(), authConfig,
 		func(ctx context.Context) error {
 			client := cli.Client()
 			_, err := client.Ping(ctx)
 			return err
-		})
-	if err != nil {
+		}); err != nil {
 		return err
 	}
-	fmt.Fprintln(cli.Out(), "To complete the activation, please restart docker with 'systemctl restart docker'")
+	fmt.Fprintln(cli.Out(), `Succesfully activated engine.
+Restart docker with 'systemctl restart docker' to complete the activation.`)
 	return nil
 }
 

diff --git a/cli/command/engine/auth.go b/cli/command/engine/auth.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/trust"
+	clitypes "github.com/docker/cli/types"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
 	registrytypes "github.com/docker/docker/api/types/registry"
@@ -13,7 +14,7 @@ import (
 
 func getRegistryAuth(cli command.Cli, registryPrefix string) (*types.AuthConfig, error) {
 	if registryPrefix == "" {
-		registryPrefix = "docker.io/store/docker"
+		registryPrefix = clitypes.RegistryPrefix
 	}
 	distributionRef, err := reference.ParseNormalizedNamed(registryPrefix)
 	if err != nil {

diff --git a/cli/command/engine/check.go b/cli/command/engine/check.go
@@ -13,10 +13,6 @@ import (
 	"github.com/spf13/cobra"
 )
 
-const (
-	releaseNotePrefix = "https://docs.docker.com/releasenotes"
-)
-
 type checkOptions struct {
 	registryPrefix string
 	preReleases    bool
@@ -39,7 +35,7 @@ func newCheckForUpdatesCommand(dockerCli command.Cli) *cobra.Command {
 		},
 	}
 	flags := cmd.Flags()
-	flags.StringVar(&options.registryPrefix, "registry-prefix", "docker.io/store/docker", "Override the existing location where engine images are pulled")
+	flags.StringVar(&options.registryPrefix, "registry-prefix", clitypes.RegistryPrefix, "Override the existing location where engine images are pulled")
 	flags.BoolVar(&options.downgrades, "downgrades", false, "Report downgrades (default omits older versions)")
 	flags.BoolVar(&options.preReleases, "pre-releases", false, "Include pre-release versions")
 	flags.BoolVar(&options.upgrades, "upgrades", true, "Report available upgrades")
@@ -52,7 +48,7 @@ func newCheckForUpdatesCommand(dockerCli command.Cli) *cobra.Command {
 
 func runCheck(dockerCli command.Cli, options checkOptions) error {
 	if !isRoot() {
-		return errors.New("must be privileged to activate engine")
+		return errors.New("this command must be run as a privileged user")
 	}
 	ctx := context.Background()
 	client := dockerCli.Client()
@@ -119,7 +115,7 @@ func processVersions(currentVersion, verType string,
 			availUpdates = append(availUpdates, clitypes.Update{
 				Type:    verType,
 				Version: ver.Tag,
-				Notes:   fmt.Sprintf("%s/%s", releaseNotePrefix, ver.Tag),
+				Notes:   fmt.Sprintf("%s/%s", clitypes.ReleaseNotePrefix, ver.Tag),
 			})
 		}
 	}

diff --git a/cli/command/engine/update.go b/cli/command/engine/update.go
@@ -6,6 +6,7 @@ import (
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
+	clitypes "github.com/docker/cli/types"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
@@ -25,27 +26,22 @@ func newUpdateCommand(dockerCli command.Cli) *cobra.Command {
 
 	flags.StringVar(&options.EngineVersion, "version", "", "Specify engine version")
 	flags.StringVar(&options.EngineImage, "engine-image", "", "Specify engine image")
-	flags.StringVar(&options.RegistryPrefix, "registry-prefix", "", "Override the current location where engine images are pulled")
+	flags.StringVar(&options.RegistryPrefix, "registry-prefix", clitypes.RegistryPrefix, "Override the current location where engine images are pulled")
 	flags.StringVar(&options.sockPath, "containerd", "", "override default location of containerd endpoint")
 
 	return cmd
 }
 
 func runUpdate(dockerCli command.Cli, options extendedEngineInitOptions) error {
 	if !isRoot() {
-		return errors.New("must be privileged to activate engine")
+		return errors.New("this command must be run as a privileged user")
 	}
 	ctx := context.Background()
 	client, err := dockerCli.NewContainerizedEngineClient(options.sockPath)
 	if err != nil {
 		return errors.Wrap(err, "unable to access local containerd")
 	}
 	defer client.Close()
-	if options.EngineImage == "" || options.RegistryPrefix == "" {
-		if options.RegistryPrefix == "" {
-			options.RegistryPrefix = "docker.io/store/docker"
-		}
-	}
 	authConfig, err := getRegistryAuth(dockerCli, options.RegistryPrefix)
 	if err != nil {
 		return err
@@ -59,6 +55,7 @@ func runUpdate(dockerCli command.Cli, options extendedEngineInitOptions) error {
 		}); err != nil {
 		return err
 	}
-	fmt.Fprintln(dockerCli.Out(), "To complete the update, please restart docker with 'systemctl restart docker'")
+	fmt.Fprintln(dockerCli.Out(), `Succesfully updated engine.
+Restart docker with 'systemctl restart docker' to complete the update.`)
 	return nil
 }
diff --git a/cli/command/engine/update_test.go b/cli/command/engine/update_test.go
@@ -28,7 +28,7 @@ func TestUpdateHappy(t *testing.T) {
 		},
 	)
 	cmd := newUpdateCommand(testCli)
-	cmd.Flags().Set("registry-prefix", "docker.io/store/docker")
+	cmd.Flags().Set("registry-prefix", clitypes.RegistryPrefix)
 	cmd.Flags().Set("version", "someversion")
 	err := cmd.Execute()
 	assert.NilError(t, err)

diff --git a/dockerfiles/Dockerfile.e2e b/dockerfiles/Dockerfile.e2e
@@ -15,28 +15,6 @@ RUN apt-get update && apt-get install -y \
     iptables \
     && rm -rf /var/lib/apt/lists/*
 
-# TODO - consider replacing with an official image and a multi-stage build to pluck the binaries out
-#ARG CONTAINERD_VERSION=v1.1.2
-#ARG CONTAINERD_VERSION=47a128d
-#ARG CONTAINERD_VERSION=6c3e782f
-ARG CONTAINERD_VERSION=65839a47a88b0a1c5dc34981f1741eccefc9f2b0
-RUN git clone https://github.com/containerd/containerd.git /go/src/github.com/containerd/containerd && \
-    cd /go/src/github.com/containerd/containerd && \
-    git checkout ${CONTAINERD_VERSION} && \
-    make && \
-    make install
-COPY e2eengine/config.toml /etc/containerd/config.toml
-COPY --from=containerd-shim-process /bin/containerd-shim-process-v1 /bin/
-
-
-# TODO - consider replacing with an official image and a multi-stage build to pluck the binaries out
-ARG RUNC_VERSION=v1.0.0-rc5
-RUN git clone https://github.com/opencontainers/runc.git /go/src/github.com/opencontainers/runc && \
-    cd /go/src/github.com/opencontainers/runc && \
-    git checkout ${RUNC_VERSION} && \
-    make && \
-    make install
-
 ARG COMPOSE_VERSION=1.21.2
 RUN curl -L https://github.com/docker/compose/releases/download/${COMPOSE_VERSION}/docker-compose-`uname -s`-`uname -m` -o /usr/local/bin/docker-compose \
     && chmod +x /usr/local/bin/docker-compose

diff --git a/e2eengine/altroot/altroot_test.go b/e2eengine/altroot/altroot_test.go
diff --git a/e2eengine/config.toml b/e2eengine/config.toml
diff --git a/e2eengine/multi/multi_test.go b/e2eengine/multi/multi_test.go
diff --git a/e2eengine/utils.go b/e2eengine/utils.go