From 5eac0740c3d57150af39a3da26fb680744213bd1 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 28 Jul 2018 06:25:23 +0200 Subject: Eliminate the "DockerArchive means pull all refPairs" special case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead, encode it explicitly in pullGoal.pullAllPairs. Should not change behavior (but does not add unit tests for all of it). Signed-off-by: Miloslav Trmač Closes: #1176 Approved by: rhatdan --- libpod/image/pull.go | 28 +++++++++++++++++++--------- libpod/image/pull_test.go | 30 ++++++++++++++++++++++++------ 2 files changed, 43 insertions(+), 15 deletions(-) (limited to 'libpod/image') diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 0d1299c81..26f18691f 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -48,7 +48,7 @@ var ( DefaultLocalRepo = "localhost" ) -// pullRefPair records a pair of prepared image references to try to pull (if not DockerArchive) or to pull all (if DockerArchive) +// pullRefPair records a pair of prepared image references to pull. type pullRefPair struct { image string srcRef types.ImageReference @@ -57,10 +57,11 @@ type pullRefPair struct { // pullGoal represents the prepared image references and decided behavior to be executed by imagePull type pullGoal struct { - refPairs []pullRefPair + refPairs []pullRefPair + pullAllPairs bool // Pull all refPairs instead of stopping on first success. } -// pullRefName records a prepared source reference and a destination name to try to pull (if not DockerArchive) or to pull all (if DockerArchive) +// pullRefName records a prepared source reference and a destination name to pull. type pullRefName struct { image string srcRef types.ImageReference @@ -69,11 +70,15 @@ type pullRefName struct { // pullGoalNames is an intermediate variant of pullGoal which uses pullRefName instead of pullRefPair. type pullGoalNames struct { - refNames []pullRefName + refNames []pullRefName + pullAllPairs bool // Pull all refNames instead of stopping on first success. } func singlePullRefNameGoal(rn pullRefName) *pullGoalNames { - return &pullGoalNames{refNames: []pullRefName{rn}} + return &pullGoalNames{ + refNames: []pullRefName{rn}, + pullAllPairs: false, // Does not really make a difference. + } } func getPullRefName(srcRef types.ImageReference, destName string) pullRefName { @@ -137,7 +142,8 @@ func pullGoalNamesFromImageReference(ctx context.Context, srcRef types.ImageRefe res = append(res, pullInfo) } return &pullGoalNames{ - refNames: res, + refNames: res, + pullAllPairs: true, }, nil case OCIArchive: @@ -241,7 +247,7 @@ func (i *Image) pullImage(ctx context.Context, writer io.Writer, authfile, signa io.WriteString(writer, "Failed\n") } } else { - if imageInfo.srcRef.Transport().Name() != DockerArchive { + if !goal.pullAllPairs { return []string{imageInfo.image}, nil } images = append(images, imageInfo.image) @@ -325,7 +331,8 @@ func pullGoalNamesFromPossiblyUnqualifiedName(inputName string) (*pullGoalNames, pullNames = append(pullNames, ps) } return &pullGoalNames{ - refNames: pullNames, + refNames: pullNames, + pullAllPairs: false, }, nil } @@ -357,5 +364,8 @@ func (ir *Runtime) pullGoalFromGoalNames(goalNames *pullGoalNames) (pullGoal, er dstRef: destRef, } } - return pullGoal{refPairs: res}, nil + return pullGoal{ + refPairs: res, + pullAllPairs: goalNames.pullAllPairs, + }, nil } diff --git a/libpod/image/pull_test.go b/libpod/image/pull_test.go index 910319de6..af8bf14a5 100644 --- a/libpod/image/pull_test.go +++ b/libpod/image/pull_test.go @@ -81,24 +81,28 @@ func TestGetPullRefName(t *testing.T) { func TestPullGoalNamesFromImageReference(t *testing.T) { type expected struct{ image, dstName string } for _, c := range []struct { - srcName string - expected []expected + srcName string + expected []expected + expectedPullAllPairs bool }{ // == docker-archive: - {"docker-archive:/dev/this-does-not-exist", nil}, // Input does not exist. - {"docker-archive:/dev/null", nil}, // Input exists but does not contain a manifest. + {"docker-archive:/dev/this-does-not-exist", nil, false}, // Input does not exist. + {"docker-archive:/dev/null", nil, false}, // Input exists but does not contain a manifest. // FIXME: The implementation has extra code for len(manifest) == 0?! That will fail in getImageDigest anyway. { // RepoTags is empty "docker-archive:testdata/docker-unnamed.tar.xz", []expected{{"@ec9293436c2e66da44edb9efb8d41f6b13baf62283ebe846468bc992d76d7951", "@ec9293436c2e66da44edb9efb8d41f6b13baf62283ebe846468bc992d76d7951"}}, + false, }, { // RepoTags is a [docker.io/library/]name:latest, normalized to the short format. "docker-archive:testdata/docker-name-only.tar.xz", []expected{{"localhost/pretty-empty:latest", "localhost/pretty-empty:latest"}}, + true, }, { // RepoTags is a registry/name:latest "docker-archive:testdata/docker-registry-name.tar.xz", []expected{{"example.com/empty:latest", "example.com/empty:latest"}}, + true, }, { // RepoTags has multiple items for a single image "docker-archive:testdata/docker-two-names.tar.xz", @@ -106,33 +110,39 @@ func TestPullGoalNamesFromImageReference(t *testing.T) { {"localhost/pretty-empty:latest", "localhost/pretty-empty:latest"}, {"example.com/empty:latest", "example.com/empty:latest"}, }, + true, }, { // FIXME: Two images in a single archive - only the "first" one (whichever it is) is returned // (and docker-archive: then refuses to read anything when the manifest has more than 1 item) "docker-archive:testdata/docker-two-images.tar.xz", []expected{{"example.com/empty:latest", "example.com/empty:latest"}}, // "example.com/empty/but:different" exists but is ignored + true, }, // == oci-archive: - {"oci-archive:/dev/this-does-not-exist", nil}, // Input does not exist. - {"oci-archive:/dev/null", nil}, // Input exists but does not contain a manifest. + {"oci-archive:/dev/this-does-not-exist", nil, false}, // Input does not exist. + {"oci-archive:/dev/null", nil, false}, // Input exists but does not contain a manifest. // FIXME: The remaining tests are commented out for now, because oci-archive: does not work unprivileged. // { // No name annotation // "oci-archive:testdata/oci-unnamed.tar.gz", // []expected{{"@5c8aca8137ac47e84c69ae93ce650ce967917cc001ba7aad5494073fac75b8b6", "@5c8aca8137ac47e84c69ae93ce650ce967917cc001ba7aad5494073fac75b8b6"}}, + // false, // }, // { // Name is a name:latest (no normalization is defined). // "oci-archive:testdata/oci-name-only.tar.gz", // []expected{{"localhost/pretty-empty:latest", "localhost/pretty-empty:latest"}}, + // false, // }, // { // Name is a registry/name:latest // "oci-archive:testdata/oci-registry-name.tar.gz", // []expected{{"example.com/empty:latest", "example.com/empty:latest"}}, + // false, // }, // { // Name exists, but is an invalid Docker reference; such names are passed through, and will fail when intepreting dstName. // "oci-archive:testdata/oci-non-docker-name.tar.gz", // []expected{{"UPPERCASE-IS-INVALID", "UPPERCASE-IS-INVALID"}}, + // false, // }, // Maybe test support of two images in a single archive? It should be transparently handled by adding a reference to srcRef. @@ -140,16 +150,19 @@ func TestPullGoalNamesFromImageReference(t *testing.T) { { // Absolute path "dir:/dev/this-does-not-exist", []expected{{"localhost/dev/this-does-not-exist", "localhost/dev/this-does-not-exist"}}, + false, }, { // Relative path, single element. // FIXME? Note the :latest difference in .image. (In .dstName as well, but it has the same semantics in there.) "dir:this-does-not-exist", []expected{{"localhost/this-does-not-exist:latest", "localhost/this-does-not-exist:latest"}}, + false, }, { // Relative path, multiple elements. // FIXME: This does not add localhost/, and dstName is parsed as docker.io/testdata. "dir:testdata/this-does-not-exist", []expected{{"testdata/this-does-not-exist", "testdata/this-does-not-exist"}}, + false, }, // == Others, notably: @@ -157,10 +170,12 @@ func TestPullGoalNamesFromImageReference(t *testing.T) { { // Fully-specified input "docker://docker.io/library/busybox:latest", []expected{{"docker://docker.io/library/busybox:latest", "docker.io/library/busybox:latest"}}, + false, }, { // Minimal form of the input "docker://busybox", []expected{{"docker://busybox", "docker.io/library/busybox:latest"}}, + false, }, // === tarball: (as an example of what happens when ImageReference.DockerReference is nil). @@ -168,6 +183,7 @@ func TestPullGoalNamesFromImageReference(t *testing.T) { // (This is NOT an API promise that the results will continue to be this way.) "tarball:/dev/null", []expected{{"tarball:/dev/null", "tarball:/dev/null"}}, + false, }, } { srcRef, err := alltransports.ParseImageName(c.srcName) @@ -182,6 +198,7 @@ func TestPullGoalNamesFromImageReference(t *testing.T) { for i, e := range c.expected { assert.Equal(t, pullRefName{image: e.image, srcRef: srcRef, dstName: e.dstName}, res.refNames[i], fmt.Sprintf("%s #%d", c.srcName, i)) } + assert.Equal(t, c.expectedPullAllPairs, res.pullAllPairs, c.srcName) } } } @@ -296,6 +313,7 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { } } assert.Equal(t, c.expected, strings, c.input) + assert.False(t, res.pullAllPairs, c.input) } } } -- cgit v1.2.3-54-g00ecf