From 8d73e456631ac283eb238584f963ee0df8c84973 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 28 Jul 2018 06:33:11 +0200 Subject: Eliminate duplicate determination whether to use search registries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of duplicating the hasRegistry logic, just record whether we did use search or not. 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 | 36 +++++++++++++++++++----------------- libpod/image/pull_test.go | 22 +++++++++++++++++----- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/libpod/image/pull.go b/libpod/image/pull.go index 26f18691f..f63a68a69 100644 --- a/libpod/image/pull.go +++ b/libpod/image/pull.go @@ -57,8 +57,9 @@ type pullRefPair struct { // pullGoal represents the prepared image references and decided behavior to be executed by imagePull type pullGoal struct { - refPairs []pullRefPair - pullAllPairs bool // Pull all refPairs instead of stopping on first success. + refPairs []pullRefPair + pullAllPairs bool // Pull all refPairs instead of stopping on first success. + usedSearchRegistries bool // refPairs construction has depended on registries.GetRegistries() } // pullRefName records a prepared source reference and a destination name to pull. @@ -70,14 +71,16 @@ type pullRefName struct { // pullGoalNames is an intermediate variant of pullGoal which uses pullRefName instead of pullRefPair. type pullGoalNames struct { - refNames []pullRefName - pullAllPairs bool // Pull all refNames instead of stopping on first success. + refNames []pullRefName + pullAllPairs bool // Pull all refNames instead of stopping on first success. + usedSearchRegistries bool // refPairs construction has depended on registries.GetRegistries() } func singlePullRefNameGoal(rn pullRefName) *pullGoalNames { return &pullGoalNames{ - refNames: []pullRefName{rn}, - pullAllPairs: false, // Does not really make a difference. + refNames: []pullRefName{rn}, + pullAllPairs: false, // Does not really make a difference. + usedSearchRegistries: false, } } @@ -142,8 +145,9 @@ func pullGoalNamesFromImageReference(ctx context.Context, srcRef types.ImageRefe res = append(res, pullInfo) } return &pullGoalNames{ - refNames: res, - pullAllPairs: true, + refNames: res, + pullAllPairs: true, + usedSearchRegistries: false, }, nil case OCIArchive: @@ -260,11 +264,7 @@ func (i *Image) pullImage(ctx context.Context, writer io.Writer, authfile, signa if err != nil { return nil, err } - hasRegistryInName, err := i.hasRegistry() - if err != nil { - return nil, err - } - if !hasRegistryInName && len(searchRegistries) == 0 { + if goal.usedSearchRegistries && len(searchRegistries) == 0 { return nil, errors.Errorf("image name provided is a short name and no search registries are defined in %s.", registryPath) } return nil, errors.Errorf("unable to find image in the registries defined in %q", registryPath) @@ -331,8 +331,9 @@ func pullGoalNamesFromPossiblyUnqualifiedName(inputName string) (*pullGoalNames, pullNames = append(pullNames, ps) } return &pullGoalNames{ - refNames: pullNames, - pullAllPairs: false, + refNames: pullNames, + pullAllPairs: false, + usedSearchRegistries: true, }, nil } @@ -365,7 +366,8 @@ func (ir *Runtime) pullGoalFromGoalNames(goalNames *pullGoalNames) (pullGoal, er } } return pullGoal{ - refPairs: res, - pullAllPairs: goalNames.pullAllPairs, + refPairs: res, + pullAllPairs: goalNames.pullAllPairs, + usedSearchRegistries: goalNames.usedSearchRegistries, }, nil } diff --git a/libpod/image/pull_test.go b/libpod/image/pull_test.go index af8bf14a5..3a914526d 100644 --- a/libpod/image/pull_test.go +++ b/libpod/image/pull_test.go @@ -199,6 +199,7 @@ func TestPullGoalNamesFromImageReference(t *testing.T) { 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) + assert.False(t, res.usedSearchRegistries, c.srcName) } } } @@ -232,37 +233,43 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { os.Setenv("REGISTRIES_CONFIG_PATH", registriesConf.Name()) for _, c := range []struct { - input string - expected []pullRefStrings + input string + expected []pullRefStrings + expectedUsedSearchRegistries bool }{ - {"#", nil}, // Clearly invalid. + {"#", nil, false}, // Clearly invalid. { // Fully-explicit docker.io, name-only. "docker.io/library/busybox", // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) []pullRefStrings{{"docker.io/library/busybox", "docker://busybox:latest", "docker.io/library/busybox"}}, + false, }, { // docker.io with implied /library/, name-only. "docker.io/busybox", // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) // The .dstName fields differ for the explicit/implicit /library/ cases, but StorageTransport.ParseStoreReference normalizes that. []pullRefStrings{{"docker.io/busybox", "docker://busybox:latest", "docker.io/busybox"}}, + false, }, { // Qualified example.com, name-only. "example.com/ns/busybox", []pullRefStrings{{"example.com/ns/busybox", "docker://example.com/ns/busybox:latest", "example.com/ns/busybox"}}, + false, }, { // Qualified example.com, name:tag. "example.com/ns/busybox:notlatest", []pullRefStrings{{"example.com/ns/busybox:notlatest", "docker://example.com/ns/busybox:notlatest", "example.com/ns/busybox:notlatest"}}, + false, }, { // Qualified example.com, name@digest. "example.com/ns/busybox" + digestSuffix, []pullRefStrings{{"example.com/ns/busybox" + digestSuffix, "docker://example.com/ns/busybox" + digestSuffix, // FIXME?! Why is .dstName dropping the digest, and adding :none?! "example.com/ns/busybox:none"}}, + false, }, // Qualified example.com, name:tag@digest. This code is happy to try, but .srcRef parsing currently rejects such input. - {"example.com/ns/busybox:notlatest" + digestSuffix, nil}, + {"example.com/ns/busybox:notlatest" + digestSuffix, nil, false}, { // Unqualified, single-name, name-only "busybox", []pullRefStrings{ @@ -270,6 +277,7 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) {"docker.io/busybox:latest", "docker://busybox:latest", "docker.io/busybox:latest"}, }, + true, }, { // Unqualified, namespaced, name-only "ns/busybox", @@ -278,6 +286,7 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { []pullRefStrings{ {"ns/busybox", "docker://ns/busybox:latest", "ns/busybox"}, }, + false, }, { // Unqualified, name:tag "busybox:notlatest", @@ -286,6 +295,7 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) {"docker.io/busybox:notlatest", "docker://busybox:notlatest", "docker.io/busybox:notlatest"}, }, + true, }, { // Unqualified, name@digest "busybox" + digestSuffix, @@ -295,9 +305,10 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { // (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".) {"docker.io/busybox:none", "docker://busybox" + digestSuffix, "docker.io/busybox:none"}, }, + true, }, // Unqualified, name:tag@digest. This code is happy to try, but .srcRef parsing currently rejects such input. - {"busybox:notlatest" + digestSuffix, nil}, + {"busybox:notlatest" + digestSuffix, nil, false}, } { res, err := pullGoalNamesFromPossiblyUnqualifiedName(c.input) if len(c.expected) == 0 { @@ -314,6 +325,7 @@ func TestPullGoalNamesFromPossiblyUnqualifiedName(t *testing.T) { } assert.Equal(t, c.expected, strings, c.input) assert.False(t, res.pullAllPairs, c.input) + assert.Equal(t, c.expectedUsedSearchRegistries, res.usedSearchRegistries, c.input) } } } -- cgit v1.2.3-54-g00ecf