Improve flow pull behavior before container creation.

 - Also improve test coverage

Signed-off-by: Zander Mackie <zmackie@gmail.com>

cli/command/container/create.go

@@ -204,37 +204,27 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig
 		}
 	}
 
-	//create the container
+	//create the container, pulling the image (or not) based on opts.pull
 	var response container.ContainerCreateCreatedBody
-	if opts.pull == PullImageMissing { // Pull image only if it does not exist locally. Default.
-		response, err = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
-
-		//if image not found try to pull it
-		if err != nil {
-			if apiclient.IsErrNotFound(err) && namedRef != nil {
-				fmt.Fprintf(stderr, "Unable to find image '%s' locally\n", reference.FamiliarString(namedRef))
-
-				// we don't want to write to stdout anything apart from container.ID
-				if err := pullImage(ctx, dockerCli, config.Image, opts.platform, stderr); err != nil {
-					return nil, err
-				}
-				if taggedRef, ok := namedRef.(reference.NamedTagged); ok && trustedRef != nil {
-					if err := image.TagTrusted(ctx, dockerCli, trustedRef, taggedRef); err != nil {
-						return nil, err
-					}
-				}
-				// Retry
-				var retryErr error
-				response, retryErr = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
-				if retryErr != nil {
-					return nil, retryErr
-				}
-			} else {
+
+	if opts.pull == PullImageAlways {
+		if err := pullImage(ctx, dockerCli, config.Image, opts.platform, stderr); err != nil {
+			return nil, err
+		}
+		if taggedRef, ok := namedRef.(reference.NamedTagged); ok && trustedRef != nil {
+			if err := image.TagTrusted(ctx, dockerCli, trustedRef, taggedRef); err != nil {
 				return nil, err
 			}
 		}
+	}
 
-	} else if opts.pull == PullImageAlways { // Always try and pull the image.
+	response, err = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
+
+	// Pull image if it does not exist locally and we have the PullImageMissing option. Default behavior.
+	if err != nil && apiclient.IsErrNotFound(err) && namedRef != nil && opts.pull == PullImageMissing {
+		fmt.Fprintf(stderr, "Unable to find image '%s' locally\n", reference.FamiliarString(namedRef))
+
+		// we don't want to write to stdout anything apart from container.ID
 		if err := pullImage(ctx, dockerCli, config.Image, opts.platform, stderr); err != nil {
 			return nil, err
 		}
@@ -243,21 +233,17 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerConfig
 				return nil, err
 			}
 		}
-		response, err = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
-		if err != nil {
-			return nil, err
-		}
-
-	} else if opts.pull == PullImageNever { // Never try and pull the image
-		response, err = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
+	}
 
-		if err != nil {
-			if apiclient.IsErrNotFound(err) && namedRef != nil {
-				fmt.Fprintf(stderr, "Unable to find image '%s' locally\nWill not pull due to '%s'", reference.FamiliarString(namedRef), opts.pull)
-			}
+	// Try to create the container again with whatever we have at this point
+	var retryErr error
+	response, retryErr = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name)
+	if retryErr != nil {
+		// At this point we won't be pulling the image
+		if apiclient.IsErrNotFound(retryErr) && namedRef != nil {
+			fmt.Fprintf(stderr, "Unable to find image '%s' locally\nWill not pull due to '%s'", reference.FamiliarString(namedRef), opts.pull)
 		}
-	} else { // We got something weird
-		return nil, errors.Errorf("Unknown pull option : %s", opts.pull)
+		return nil, retryErr
 	}
 
 	for _, warning := range response.Warnings {

cli/command/container/create_test.go

@@ -168,6 +168,63 @@ func TestCreateContainerNeverPullsImage(t *testing.T) {
 	assert.Check(t, is.Contains(stderr, "Unable to find image 'does-not-exist-locally:latest' locally"))
 }
 
+func TestCreateContainerAlwaysPullsImage(t *testing.T) {
+	imageName := "does-not-exist-locally"
+	responseCounter := 0
+	imageCreateCounter := 0
+	containerID := "abcdef"
+
+	client := &fakeClient{
+		createContainerFunc: func(
+			config *container.Config,
+			hostConfig *container.HostConfig,
+			networkingConfig *network.NetworkingConfig,
+			containerName string,
+		) (container.ContainerCreateCreatedBody, error) {
+			defer func() { responseCounter++ }()
+			switch responseCounter {
+			case 0:
+				return container.ContainerCreateCreatedBody{ID: containerID}, nil
+			case 1:
+				return container.ContainerCreateCreatedBody{ID: containerID}, nil
+			default:
+				return container.ContainerCreateCreatedBody{}, errors.New("unexpected")
+			}
+		},
+		imageCreateFunc: func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) {
+			defer func() { imageCreateCounter++ }()
+			switch imageCreateCounter {
+			case 0:
+				return ioutil.NopCloser(strings.NewReader("")), nil
+			default:
+				return ioutil.NopCloser(strings.NewReader("")), errors.New("unexpected")
+			}
+		},
+		infoFunc: func() (types.Info, error) {
+			return types.Info{IndexServerAddress: "http://indexserver"}, nil
+		},
+	}
+	cli := test.NewFakeCli(client)
+	config := &containerConfig{
+		Config: &container.Config{
+			Image: imageName,
+		},
+		HostConfig: &container.HostConfig{},
+	}
+	body, err := createContainer(context.Background(), cli, config, &createOptions{
+		name:      "name",
+		platform:  runtime.GOOS,
+		untrusted: true,
+		pull:      PullImageAlways,
+	})
+	assert.NilError(t, err)
+	expected := container.ContainerCreateCreatedBody{}
+	assert.Check(t, is.DeepEqual(expected, *body))
+	stderr := cli.ErrBuffer().String()
+	assert.Check(t, is.Contains(stderr, "Unable to find image 'does-not-exist-locally:latest' locally"))
+	assert.Check(t, is.Equal(imageCreateCounter, 1))
+}
+
 func TestNewCreateCommandWithContentTrustErrors(t *testing.T) {
 	testCases := []struct {
 		name          string