Reorganized to get details about a cached artifact

Get details about an artifact first (if it needs to be rebuilt, retagged
or pushed), and then execute. Also added a unit test for this.

Author: Priya Wadhwa

pkg/skaffold/build/cache.go

@@ -35,7 +35,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/google/go-containerregistry/pkg/name"
-	"github.com/google/go-containerregistry/pkg/v1/remote"
 	homedir "github.com/mitchellh/go-homedir"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -63,13 +62,10 @@ var (
 	// For testing
 	hashForArtifact = getHashForArtifact
 	localCluster    = config.GetLocalCluster
+	remoteDigest    = docker.RemoteDigest
 	noCache         = &Cache{}
 )
 
-const (
-	prefixLength = len("sha256:")
-)
-
 // NewCache returns the current state of the cache
 func NewCache(useCache bool, cacheFile string) *Cache {
 	if !useCache {
@@ -132,7 +128,7 @@ func (c *Cache) RetrieveCachedArtifacts(ctx context.Context, out io.Writer, arti
 	var needToBuild []*latest.Artifact
 	var built []Artifact
 	for _, a := range artifacts {
-		artifact, err := c.retrieveCachedArtifact(ctx, out, a)
+		artifact, err := c.resolveCachedArtifact(ctx, out, a)
 		if err != nil {
 			logrus.Debugf("error retrieving cached artifact for %s: %v\n", a.ImageName, err)
 			needToBuild = append(needToBuild, a)
@@ -147,40 +143,67 @@ func (c *Cache) RetrieveCachedArtifacts(ctx context.Context, out io.Writer, arti
 	return needToBuild, built
 }
 
-func (c *Cache) retrieveCachedArtifact(ctx context.Context, out io.Writer, a *latest.Artifact) (*Artifact, error) {
+func (c *Cache) resolveCachedArtifact(ctx context.Context, out io.Writer, a *latest.Artifact) (*Artifact, error) {
+	details, err := c.retrieveCachedArtifactDetails(ctx, out, a)
+	if err != nil {
+		return nil, errors.Wrap(err, "getting cached artifact details")
+	}
+	if details.needsRebuild {
+		return nil, nil
+	}
+	color.Green.Fprintf(out, "Found %s locally, retagging and pushing if necessary ...\n", a.ImageName)
+	if details.needsRetag {
+		if err := c.client.Tag(ctx, details.prebuiltImage, details.hashTag); err != nil {
+			return nil, errors.Wrap(err, "retagging image")
+		}
+	}
+	if details.needsPush {
+		if _, err := c.client.Push(ctx, out, details.hashTag); err != nil {
+			return nil, errors.Wrap(err, "pushing image")
+		}
+	}
+	color.Green.Fprintf(out, "Resolved %s, skipping rebuild.\n", details.hashTag)
+	return &Artifact{
+		ImageName: a.ImageName,
+		Tag:       details.hashTag,
+	}, nil
+}
+
+type cachedArtifactDetails struct {
+	needsRebuild  bool
+	needsRetag    bool
+	needsPush     bool
+	prebuiltImage string
+	hashTag       string
+}
+
+func (c *Cache) retrieveCachedArtifactDetails(ctx context.Context, out io.Writer, a *latest.Artifact) (*cachedArtifactDetails, error) {
 	hash, err := hashForArtifact(ctx, a)
 	if err != nil {
 		return nil, errors.Wrapf(err, "getting hash for artifact %s", a.ImageName)
 	}
 	a.WorkspaceHash = hash
+	localCluster, _ := localCluster()
 	imageDetails, cacheHit := c.artifactCache[hash]
 	if !cacheHit {
-		return nil, nil
+		return &cachedArtifactDetails{
+			needsRebuild: true,
+		}, nil
 	}
 	hashTag := fmt.Sprintf("%s:%s", a.ImageName, hash)
 
 	// Check if we are using a local cluster
-	local, _ := localCluster()
 	var existsRemotely bool
-	if !local {
+	if !localCluster {
 		// Check if tagged image exists remotely with the same digest
 		existsRemotely = imageExistsRemotely(hashTag, imageDetails.Digest)
 	}
 
 	// See if this image exists in the local daemon
 	if c.client.ImageExists(ctx, hashTag) {
-		color.Green.Fprintf(out, "Found %s locally...\n", a.ImageName)
-		// Push if the image doesn't exist remotely
-		if !existsRemotely && !local {
-			color.Green.Fprintf(out, "Pushing %s since it doesn't exist remotely...\n", a.ImageName)
-			if _, err := c.client.Push(ctx, out, hashTag); err != nil {
-				return nil, errors.Wrapf(err, "pushing %s", hashTag)
-			}
-		}
-		color.Green.Fprintf(out, "%s ready, skipping rebuild\n", hashTag)
-		return &Artifact{
-			ImageName: a.ImageName,
-			Tag:       hashTag,
+		return &cachedArtifactDetails{
+			needsPush: !existsRemotely && !localCluster,
+			hashTag:   hashTag,
 		}, nil
 	}
 	// Check for a local image with the same digest as the image we want to build
@@ -191,20 +214,12 @@ func (c *Cache) retrieveCachedArtifact(ctx context.Context, out io.Writer, a *la
 	if prebuiltImage == "" {
 		return nil, errors.New("no tagged prebuilt image")
 	}
-	color.Green.Fprintf(out, "Found %s locally, retagging and pushing...\n", a.ImageName)
-	// Retag the image
-	if err := c.client.Tag(ctx, prebuiltImage, hashTag); err != nil {
-		return nil, errors.Wrap(err, "retagging image")
-	}
-	// Push the retagged image
-	if _, err := c.client.Push(ctx, out, hashTag); err != nil {
-		return nil, errors.Wrap(err, "pushing image")
-	}
 
-	color.Green.Fprintf(out, "Retagged %s, skipping rebuild.\n", prebuiltImage)
-	return &Artifact{
-		ImageName: a.ImageName,
-		Tag:       hashTag,
+	return &cachedArtifactDetails{
+		needsRetag:    true,
+		needsPush:     !localCluster,
+		prebuiltImage: prebuiltImage,
+		hashTag:       hashTag,
 	}, nil
 }
 
@@ -230,21 +245,15 @@ func (c *Cache) retrievePrebuiltImage(ctx context.Context, details ImageDetails)
 
 func imageExistsRemotely(image, digest string) bool {
 	if digest == "" {
+		logrus.Debugf("Checking if %s exists remotely, but digest is empty", image)
 		return false
 	}
-	ref, err := name.ParseReference(image, name.WeakValidation)
-	if err != nil {
-		return false
-	}
-	img, err := remote.Image(ref)
-	if err != nil {
-		return false
-	}
-	d, err := img.Digest()
+	d, err := remoteDigest(image)
 	if err != nil {
+		logrus.Debugf("Checking if %s exists remotely, can't get digest: %v", image, err)
 		return false
 	}
-	return d.Hex == digest[prefixLength:]
+	return d == digest
 }
 
 // CacheArtifacts determines the hash for each artifact, stores it in the artifact cache, and saves the cache at the end

pkg/skaffold/build/cache_test.go

@@ -208,3 +208,165 @@ func createTempCacheFile(t *testing.T, cacheFileContents interface{}) string {
 	}
 	return temp.Name()
 }
+
+func TestRetrieveCachedArtifactDetails(t *testing.T) {
+	tests := []struct {
+		name         string
+		localCluster bool
+		artifact     *latest.Artifact
+		hashes       map[string]string
+		digest       string
+		api          testutil.FakeAPIClient
+		cache        *Cache
+		expected     *cachedArtifactDetails
+	}{
+		{
+			name:     "image doesn't exist in cache, remote cluster",
+			artifact: &latest.Artifact{ImageName: "image"},
+			hashes:   map[string]string{"image": "hash"},
+			cache:    noCache,
+			expected: &cachedArtifactDetails{
+				needsRebuild: true,
+			},
+		},
+		{
+			name:         "image doesn't exist in cache, local cluster",
+			artifact:     &latest.Artifact{ImageName: "image"},
+			hashes:       map[string]string{"image": "hash"},
+			localCluster: true,
+			cache:        noCache,
+			expected: &cachedArtifactDetails{
+				needsRebuild: true,
+			},
+		},
+		{
+			name:     "image in cache and exists remotely, remote cluster",
+			artifact: &latest.Artifact{ImageName: "image"},
+			hashes:   map[string]string{"image": "hash"},
+			api: testutil.FakeAPIClient{
+				TagToImageID: map[string]string{"image:hash": "image:tag"},
+				ImageSummaries: []types.ImageSummary{
+					{
+						RepoDigests: []string{"digest"},
+						RepoTags:    []string{"image:hash"},
+					},
+				},
+			},
+			cache: &Cache{
+				useCache:      true,
+				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: "digest"}},
+			},
+			digest: "digest",
+			expected: &cachedArtifactDetails{
+				hashTag: "image:hash",
+			},
+		},
+		{
+			name:         "image in cache and exists in daemon, local cluster",
+			artifact:     &latest.Artifact{ImageName: "image"},
+			hashes:       map[string]string{"image": "hash"},
+			localCluster: true,
+			api: testutil.FakeAPIClient{
+				TagToImageID: map[string]string{"image:hash": "image:tag"},
+				ImageSummaries: []types.ImageSummary{
+					{
+						RepoDigests: []string{"digest"},
+						RepoTags:    []string{"image:hash"},
+					},
+				},
+			},
+			cache: &Cache{
+				useCache:      true,
+				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: "digest"}},
+			},
+			digest: "digest",
+			expected: &cachedArtifactDetails{
+				hashTag: "image:hash",
+			},
+		},
+		{
+			name:     "image in cache, prebuilt image exists, remote cluster",
+			artifact: &latest.Artifact{ImageName: "image"},
+			hashes:   map[string]string{"image": "hash"},
+			api: testutil.FakeAPIClient{
+				ImageSummaries: []types.ImageSummary{
+					{
+						RepoDigests: []string{"digest"},
+						RepoTags:    []string{"anotherimage:hash"},
+					},
+				},
+			},
+			cache: &Cache{
+				useCache:      true,
+				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: "digest"}},
+			},
+			digest: "digest",
+			expected: &cachedArtifactDetails{
+				needsRetag:    true,
+				needsPush:     true,
+				prebuiltImage: "anotherimage:hash",
+				hashTag:       "image:hash",
+			},
+		},
+		{
+			name:         "image in cache, prebuilt image exists, local cluster",
+			artifact:     &latest.Artifact{ImageName: "image"},
+			hashes:       map[string]string{"image": "hash"},
+			localCluster: true,
+			api: testutil.FakeAPIClient{
+				ImageSummaries: []types.ImageSummary{
+					{
+						RepoDigests: []string{"digest"},
+						RepoTags:    []string{"anotherimage:hash"},
+					},
+				},
+			},
+			cache: &Cache{
+				useCache:      true,
+				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: "digest"}},
+			},
+			digest: "digest",
+			expected: &cachedArtifactDetails{
+				needsRetag:    true,
+				prebuiltImage: "anotherimage:hash",
+				hashTag:       "image:hash",
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			originalHash := hashForArtifact
+			hashForArtifact = mockHashForArtifact(test.hashes)
+			defer func() {
+				hashForArtifact = originalHash
+			}()
+
+			originalLocal := localCluster
+			localCluster = func() (bool, error) {
+				return test.localCluster, nil
+			}
+			defer func() {
+				localCluster = originalLocal
+			}()
+
+			originalRemoteDigest := remoteDigest
+			remoteDigest = func(string) (string, error) {
+				return test.digest, nil
+			}
+			defer func() {
+				remoteDigest = originalRemoteDigest
+			}()
+
+			test.cache.client = docker.NewLocalDaemon(&test.api, nil)
+			actual, err := test.cache.retrieveCachedArtifactDetails(context.Background(), os.Stdout, test.artifact)
+			if err != nil {
+				t.Fatalf("error retrieving artifact details: %v", err)
+			}
+			// cmp.Diff cannot access unexported fields, so use reflect.DeepEqual here directly
+			if !reflect.DeepEqual(test.expected, actual) {
+				t.Errorf("Expected: %v, Actual: %v", test.expected, actual)
+			}
+		})
+	}
+}