From 965664d89bbe0d09028381039df8902c1787dbe8 Mon Sep 17 00:00:00 2001 From: Zander Mackie Date: Sat, 16 Feb 2019 18:41:56 -0500 Subject: [PATCH] Improve flow pull behavior before container creation. - Also improve test coverage Signed-off-by: Zander Mackie --- cli/command/container/create.go | 68 +++++++++++----------------- cli/command/container/create_test.go | 49 ++++++++++++++++++-- 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/cli/command/container/create.go b/cli/command/container/create.go index a41c92d7ae..e2043425f7 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -223,37 +223,10 @@ 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 { - return nil, err - } - } - - } else if opts.pull == PullImageAlways { // Always try and pull the image. + if opts.pull == PullImageAlways { if err := pullImage(ctx, dockerCli, config.Image, opts.platform, stderr); err != nil { return nil, err } @@ -262,21 +235,32 @@ 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 { + } + + response, err = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name) + + if err != nil { + // Pull image if it does not exist locally and we have the PullImageMissing option. Default behavior. + if 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 + } + if taggedRef, ok := namedRef.(reference.NamedTagged); ok && trustedRef != nil { + if err := image.TagTrusted(ctx, dockerCli, trustedRef, taggedRef); err != nil { + return nil, err + } + } + + var retryErr error + response, retryErr = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, opts.name) + if retryErr != nil { + return nil, retryErr + } + } else { 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) - } - } - } else { // We got something weird - return nil, errors.Errorf("Unknown pull option : %s", opts.pull) } for _, warning := range response.Warnings { diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index 4db851e8fc..539124f78f 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -158,17 +158,58 @@ func TestCreateContainerNeverPullsImage(t *testing.T) { }, HostConfig: &container.HostConfig{}, } - body, err := createContainer(context.Background(), cli, config, &createOptions{ + _, err := createContainer(context.Background(), cli, config, &createOptions{ name: "name", platform: runtime.GOOS, untrusted: true, pull: PullImageNever, }) + assert.ErrorContains(t, err, "fake not found") +} + +func TestCreateContainerAlwaysPullsImage(t *testing.T) { + imageName := "does-not-exist-locally" + responseCounter := 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 + default: + return container.ContainerCreateCreatedBody{}, errors.New("unexpected") + } + }, + imageCreateFunc: func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) { + return ioutil.NopCloser(strings.NewReader("")), nil + }, + 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{} + expected := container.ContainerCreateCreatedBody{ID: containerID} 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")) } func TestNewCreateCommandWithContentTrustErrors(t *testing.T) {