Replace getPullRefName by Runtime.getPullRefPair

This more or less reverts 9c9401a96c0b7d43dcea19c2972ef9612cc0a136
"Replace getPullRefPair with getPullRefName"; now that tests don't require
us to use pullRefName, move creation of storage references deeper into the
calls stack to reduce string use.

ir.getSinglePullRefNameGoal is accordingly updated to ir.getSinglePullRefPairGoal,
and we need to add a ~duplicate singlePullRefPairGoal; that duplication
of singlePullRefNameGoal will soon be resolved by dropping singlePullRefNameGoal.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Closes: #1198
Approved by: mheon

libpod/image/pull.go

@@ -63,6 +63,16 @@ type pullGoal struct {
 	searchedRegistries   []string // The list of search registries used; set only if usedSearchRegistries
 }
 
+// singlePullRefPairGoal returns a no-frills pull goal for the specified reference pair.
+func singlePullRefPairGoal(rp pullRefPair) *pullGoal {
+	return &pullGoal{
+		refPairs:             []pullRefPair{rp},
+		pullAllPairs:         false, // Does not really make a difference.
+		usedSearchRegistries: false,
+		searchedRegistries:   nil,
+	}
+}
+
 // pullRefName records a prepared source reference and a destination name to pull.
 type pullRefName struct {
 	image   string
@@ -87,7 +97,7 @@ func singlePullRefNameGoal(rn pullRefName) *pullGoalNames {
 	}
 }
 
-func getPullRefName(srcRef types.ImageReference, destName string) pullRefName {
+func (ir *Runtime) getPullRefPair(srcRef types.ImageReference, destName string) (pullRefPair, error) {
 	imgPart, err := decompose(destName)
 	if err == nil && !imgPart.hasRegistry {
 		// If the image doesn't have a registry, set it as the default repo
@@ -100,22 +110,24 @@ func getPullRefName(srcRef types.ImageReference, destName string) pullRefName {
 	if srcRef.DockerReference() != nil {
 		reference = srcRef.DockerReference().String()
 	}
-	return pullRefName{
+	destRef, err := is.Transport.ParseStoreReference(ir.store, reference)
-		image:   destName,
+	if err != nil {
-		srcRef:  srcRef,
+		return pullRefPair{}, errors.Wrapf(err, "error parsing dest reference name %#v", destName)
-		dstName: reference,
 	}
+	return pullRefPair{
+		image:  destName,
+		srcRef: srcRef,
+		dstRef: destRef,
+	}, nil
 }
 
-// getSinglePullRefNameGoal calls getPullRefName with the specified parameters, and returns a single-pair goal for the return value.
+// getSinglePullRefPairGoal calls getPullRefPair with the specified parameters, and returns a single-pair goal for the return value.
-func (ir *Runtime) getSinglePullRefNameGoal(srcRef types.ImageReference, destName string) (*pullGoal, error) {
+func (ir *Runtime) getSinglePullRefPairGoal(srcRef types.ImageReference, destName string) (*pullGoal, error) {
-	rn := getPullRefName(srcRef, destName)
+	rp, err := ir.getPullRefPair(srcRef, destName)
-	goalNames := singlePullRefNameGoal(rn)
-	goal, err := ir.pullGoalFromGoalNames(goalNames)
 	if err != nil {
 		return nil, err
 	}
-	return &goal, nil
+	return singlePullRefPairGoal(rp), nil
 }
 
 // pullGoalFromImageReference returns a pull goal for a single ImageReference, depending on the used transport.
@@ -140,7 +152,7 @@ func (ir *Runtime) pullGoalFromImageReference(ctx context.Context, srcRef types.
 			if err != nil {
 				return nil, err
 			}
-			return ir.getSinglePullRefNameGoal(srcRef, reference)
+			return ir.getSinglePullRefPairGoal(srcRef, reference)
 		}
 
 		if len(manifest[0].RepoTags) == 0 {
@@ -149,25 +161,24 @@ func (ir *Runtime) pullGoalFromImageReference(ctx context.Context, srcRef types.
 			if err != nil {
 				return nil, err
 			}
-			return ir.getSinglePullRefNameGoal(srcRef, digest)
+			return ir.getSinglePullRefPairGoal(srcRef, digest)
 		}
 
 		// Need to load in all the repo tags from the manifest
-		res := []pullRefName{}
+		res := []pullRefPair{}
 		for _, dst := range manifest[0].RepoTags {
-			pullInfo := getPullRefName(srcRef, dst)
+			pullInfo, err := ir.getPullRefPair(srcRef, dst)
+			if err != nil {
+				return nil, err
+			}
 			res = append(res, pullInfo)
 		}
-		goal, err := ir.pullGoalFromGoalNames(&pullGoalNames{
+		return &pullGoal{
-			refNames:             res,
+			refPairs:             res,
 			pullAllPairs:         true,
 			usedSearchRegistries: false,
 			searchedRegistries:   nil,
-		})
+		}, nil
-		if err != nil {
-			return nil, err
-		}
-		return &goal, nil
 
 	case OCIArchive:
 		// retrieve the manifest from index.json to access the image name
@@ -187,7 +198,7 @@ func (ir *Runtime) pullGoalFromImageReference(ctx context.Context, srcRef types.
 		} else {
 			dest = manifest.Annotations["org.opencontainers.image.ref.name"]
 		}
-		return ir.getSinglePullRefNameGoal(srcRef, dest)
+		return ir.getSinglePullRefPairGoal(srcRef, dest)
 
 	case DirTransport:
 		path := srcRef.StringWithinTransport()
@@ -198,10 +209,10 @@ func (ir *Runtime) pullGoalFromImageReference(ctx context.Context, srcRef types.
 			// so docker.io isn't prepended, and the path becomes the repository
 			image = DefaultLocalRepo + image
 		}
-		return ir.getSinglePullRefNameGoal(srcRef, image)
+		return ir.getSinglePullRefPairGoal(srcRef, image)
 
 	default:
-		return ir.getSinglePullRefNameGoal(srcRef, imgName)
+		return ir.getSinglePullRefPairGoal(srcRef, imgName)
 	}
 }
 

libpod/image/pull_test.go

@@ -61,16 +61,18 @@ func storageReferenceWithoutLocation(ref types.ImageReference) string {
 	return res
 }
 
-func TestGetPullRefName(t *testing.T) {
+func TestGetPullRefPair(t *testing.T) {
 	const imageID = "@0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
 	const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
 
+	ir, cleanup := newTestRuntime(t)
+	defer cleanup()
+
 	for _, c := range []struct{ srcName, destName, expectedImage, expectedDstName string }{
 		// == Source does not have a Docker reference (as is the case for docker-archive:, oci-archive, dir:); destination formats:
 		{ // registry/name, no tag:
 			"dir:/dev/this-does-not-exist", "example.com/from-directory",
-			// The destName value will be interpreted as "example.com/from-directory:latest" by storageTransport.
+			"example.com/from-directory", "example.com/from-directory:latest",
-			"example.com/from-directory", "example.com/from-directory",
 		},
 		{ // name, no registry, no tag:
 			"dir:/dev/this-does-not-exist", "from-directory",
@@ -98,7 +100,7 @@ func TestGetPullRefName(t *testing.T) {
 		{ // ns/name:tag, no registry:
 			// FIXME: This is interpreted as "registry == ns"
 			"dir:/dev/this-does-not-exist", "ns/from-directory:notlatest",
-			"ns/from-directory:notlatest", "ns/from-directory:notlatest",
+			"ns/from-directory:notlatest", "docker.io/ns/from-directory:notlatest",
 		},
 		{ // containers-storage image ID
 			"dir:/dev/this-does-not-exist", imageID,
@@ -116,13 +118,22 @@ func TestGetPullRefName(t *testing.T) {
 			"docker://busybox", "docker://busybox:destination",
 			"docker://busybox:destination", "docker.io/library/busybox:latest",
 		},
+		// == Invalid destination format.
+		{"tarball:/dev/null", "tarball:/dev/null", "", ""},
 	} {
+		testDescription := fmt.Sprintf("%#v %#v", c.srcName, c.destName)
 		srcRef, err := alltransports.ParseImageName(c.srcName)
-		require.NoError(t, err, c.srcName)
+		require.NoError(t, err, testDescription)
 
-		res := getPullRefName(srcRef, c.destName)
+		res, err := ir.getPullRefPair(srcRef, c.destName)
-		assert.Equal(t, pullRefName{image: c.expectedImage, srcRef: srcRef, dstName: c.expectedDstName}, res,
+		if c.expectedDstName == "" {
-			fmt.Sprintf("%#v %#v", c.srcName, c.destName))
+			assert.Error(t, err, testDescription)
+		} else {
+			require.NoError(t, err, testDescription)
+			assert.Equal(t, c.expectedImage, res.image, testDescription)
+			assert.Equal(t, srcRef, res.srcRef, testDescription)
+			assert.Equal(t, c.expectedDstName, storageReferenceWithoutLocation(res.dstRef), testDescription)
+		}
 	}
 }