Code/UX review comments

Signed-off-by: Daniel Hiltgen <daniel.hiltgen@docker.com>

cli/command/engine/activate.go

@@ -58,7 +58,7 @@ https://hub.docker.com/ then specify the file with the '--license' flag.
 	flags := cmd.Flags()
 
 	flags.StringVarP(&options.licenseFile, "license", "l", "", "License File")
-	flags.StringVarP(&options.engineVersion, "engine-version", "", "", "Specify engine version (default is to use existing version)")
+	flags.StringVarP(&options.engineVersion, "version", "", "", "Specify engine version (default is to use existing version)")
 	flags.StringVarP(&options.registryPrefix, "registry-prefix", "", "docker.io/docker", "Override the default location where engine images are pulled")
 	flags.StringVarP(&options.format, "format", "", "", "Pretty-print licenses using a Go template")
 	flags.BoolVarP(&options.quiet, "queit", "q", false, "Only display available licenses by ID")
@@ -121,11 +121,11 @@ func getLicenses(ctx context.Context, cli command.Cli, options activateOptions)
 				fmt.Fprintf(cli.Out(), "%d\t%s\n", i+1, org.Orgname)
 			}
 			fmt.Fprintf(cli.Out(), "Please choose where to generate the trial:")
-			num, err := strconv.Atoi(readInput(cli.In(), cli.Out()))
+			num, err := strconv.ParseUint(readInput(cli.In(), cli.Out()), 10, 32)
 			if err != nil {
 				return nil, err
 			}
-			if num < 0 || num > len(user.Orgs) {
+			if int(num) > len(user.Orgs) {
 				return nil, fmt.Errorf("number out of range 0-%d", len(user.Orgs))
 			}
 			if num > 0 {

cli/command/engine/init.go

@@ -19,7 +19,6 @@ type initOptions struct {
 	version        string
 	image          string
 	registryPrefix string
-	rootDir        string
 	configFile     string
 }
 
@@ -40,10 +39,9 @@ file on the host and may be pre-created before running the 'init' command.
 		},
 	}
 	flags := cmd.Flags()
-	flags.StringVarP(&options.version, "engine-version", "", defaultEngineVersion, "Specify engine version")
+	flags.StringVarP(&options.version, "version", "", defaultEngineVersion, "Specify engine version")
 	flags.StringVarP(&options.image, "engine-image", "", containerdutils.CommunityEngineImage, "Specify engine image")
 	flags.StringVarP(&options.registryPrefix, "registry-prefix", "", "docker.io/docker", "Override the default location where engine images are pulled")
-	flags.StringVarP(&options.rootDir, "root-dir", "", "/var/lib/docker", "Specify the location of the Docker Root Dir on the host")
 	flags.StringVarP(&options.configFile, "config-file", "", "/etc/docker/daemon.json", "Specify the location of the daemon configuration file on the host")
 
 	return cmd
@@ -57,5 +55,5 @@ func runInit(dockerCli command.Cli, options initOptions) error {
 		return fmt.Errorf("Unable to access local containerd: %s", err)
 	}
 	defer cclient.Close()
-	return containerdutils.InitEngine(ctx, dockerCli, dclient, cclient, options.image, options.version, options.registryPrefix, options.rootDir, options.configFile)
+	return containerdutils.InitEngine(ctx, dockerCli, dclient, cclient, options.image, options.version, options.registryPrefix, options.configFile)
 }

cli/command/engine/update.go

@@ -29,7 +29,7 @@ func newUpdateCommand(dockerCli command.Cli) *cobra.Command {
 	}
 	flags := cmd.Flags()
 
-	flags.StringVarP(&options.version, "engine-version", "", "", "Specify engine version")
+	flags.StringVarP(&options.version, "version", "", "", "Specify engine version")
 	flags.StringVarP(&options.registryPrefix, "registry-prefix", "", "docker.io/docker", "Override the default location where engine images are pulled")
 
 	return cmd

cli/containerdutils/containerd.go

@@ -48,12 +48,8 @@ func PullWithAuth(cli command.Cli, client WrappedContainerdClient, imageName str
 		return fmt.Errorf("Failed to get imgRefAndAuth: %s", err)
 	}
 	authConfig := imgRefAndAuth.AuthConfig()
-	//fmt.Printf("XXX AuthConfig: %#v\n", authConfig)
-
 	resolver := docker.NewResolver(docker.ResolverOptions{
 		Credentials: func(string) (string, string, error) {
-			//fmt.Printf("XXX in Credentials helper\n")
-			// returns username/password or ""/token
 			return authConfig.Username, authConfig.Password, nil
 		},
 	})
@@ -72,7 +68,6 @@ func PullWithAuth(cli command.Cli, client WrappedContainerdClient, imageName str
 func CheckForImage(ctx context.Context, client WrappedContainerdClient, imageName string) (bool, containerd.Image, error) {
 	image, err := client.GetImage(ctx, imageName)
 	if err == nil {
-		fmt.Printf("%s already present in containerd.\n", imageName)
 		return true, image, nil
 	}
 	if errdefs.IsNotFound(err) {

cli/containerdutils/engine.go

@@ -17,7 +17,7 @@ import (
 
 // InitEngine is the main entrypoint for `docker engine init`
 func InitEngine(ctx context.Context, dockerCli command.Cli, dclient WrappedDockerClient, cclient WrappedContainerdClient,
-	engineImage, engineVersion, registryPrefix, rootDir, configFile string) error {
+	engineImage, engineVersion, registryPrefix, configFile string) error {
 
 	// Verify engine isn't already running
 	engine, _ := GetEngine(cclient)
@@ -41,7 +41,7 @@ func InitEngine(ctx context.Context, dockerCli command.Cli, dclient WrappedDocke
 	}
 
 	// Spin up the engine
-	err = StartEngineOnContainerd(ctx, cclient, imageName, rootDir, configFile)
+	err = StartEngineOnContainerd(ctx, cclient, imageName, configFile)
 	if err != nil {
 		return fmt.Errorf("Failed to create docker daemon: %s", err)
 	}
@@ -85,43 +85,33 @@ func GetEngineImage(engine containerd.Container) (string, error) {
 	return imageName, nil
 }
 
-// GetEngineSettings will extract key settings from the current engine definition
-func GetEngineSettings(ctx context.Context, engine containerd.Container) (EngineSettings, error) {
-	ret := EngineSettings{}
+// GetEngineConfigFilePath will extract the config file location from the engine flags
+func GetEngineConfigFilePath(ctx context.Context, engine containerd.Container) (string, error) {
 	spec, err := engine.Spec(ctx)
+	configFile := ""
 	if err != nil {
-		return ret, err
+		return configFile, err
 	}
 	for i := 0; i < len(spec.Process.Args); i++ {
 		arg := spec.Process.Args[i]
-		if strings.HasPrefix(arg, "--exec-root") {
+		if strings.HasPrefix(arg, "--config-file") {
 			if strings.Contains(arg, "=") {
 				split := strings.SplitN(arg, "=", 2)
-				ret.DockerRootDir = split[1]
+				configFile = split[1]
 			} else {
 				if i+1 >= len(spec.Process.Args) {
-					return ret, fmt.Errorf("malformed --exec-root param on engine")
+					return configFile, fmt.Errorf("malformed --config-file param on engine")
 				}
-				ret.DockerRootDir = spec.Process.Args[i+1]
-			}
-		} else if strings.HasPrefix(arg, "--config-file") {
-			if strings.Contains(arg, "=") {
-				split := strings.SplitN(arg, "=", 2)
-				ret.ConfigFile = split[1]
-			} else {
-				if i+1 >= len(spec.Process.Args) {
-					return ret, fmt.Errorf("malformed --config-file param on engine")
-				}
-				ret.ConfigFile = spec.Process.Args[i+1]
+				configFile = spec.Process.Args[i+1]
 			}
 		}
 	}
 
-	if ret.DockerRootDir == "" || ret.ConfigFile == "" {
+	if configFile == "" {
 		// TODO - any more diagnostics to offer?
-		return ret, fmt.Errorf("Unable to lookup existing engine configuration")
+		return configFile, fmt.Errorf("Unable to lookup existing engine configuration")
 	}
-	return ret, nil
+	return configFile, nil
 }
 
 var (
@@ -206,7 +196,7 @@ func RemoveEngine(ctx context.Context, engine containerd.Container) error {
 }
 
 // StartEngineOnContainerd creates a new docker engine running on containerd
-func StartEngineOnContainerd(ctx context.Context, cclient WrappedContainerdClient, imageName, rootDir, configFile string) error {
+func StartEngineOnContainerd(ctx context.Context, cclient WrappedContainerdClient, imageName, configFile string) error {
 	present, image, err := CheckForImage(ctx, cclient, imageName)
 	if err != nil {
 		return fmt.Errorf("Failed to check for engine image: %s", err)
@@ -217,6 +207,12 @@ func StartEngineOnContainerd(ctx context.Context, cclient WrappedContainerdClien
 
 	ctx = namespaces.WithNamespace(ctx, EngineNamespace)
 
+	// Make sure we have a valid config file
+	rootDir, err := VerifyDockerConfig(ctx, cclient, configFile, image)
+	if err != nil {
+		return err
+	}
+
 	// Append the engine dir
 	EngineSpec.Mounts = append(EngineSpec.Mounts,
 		specs.Mount{
@@ -244,12 +240,10 @@ func StartEngineOnContainerd(ctx context.Context, cclient WrappedContainerdClien
 	)
 
 	EngineSpec.Process.Args = append(EngineSpec.Process.Args,
-		"--exec-root", rootDir,
 		"--config-file", configFile,
 	)
 
 	// Make sure all the host paths exist
-	//logrus.Infof("XXX Verifying all required host paths exist")
 	for _, mount := range EngineSpec.Mounts {
 		if mount.Type != "bind" {
 			continue
@@ -263,12 +257,6 @@ func StartEngineOnContainerd(ctx context.Context, cclient WrappedContainerdClien
 		}
 	}
 
-	// Make sure we have a valid config file too
-	err = VerifyDockerConfig(ctx, cclient, configFile, image)
-	if err != nil {
-		return err
-	}
-
 	cOpts := []containerd.NewContainerOpts{
 		containerd.WithNewSnapshot(EngineContainerName, image),
 		restart.WithStatus(containerd.Running),