Merge pull request #3591 from dgageot/simpler-artifact-hasher

Simpler artifact hasher
GoogleContainerTools · Jan 27, 2020 · 8d1dc4e · 8d1dc4e
2 parents bcd903a + 217d00b
commit 8d1dc4e
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 21 deletions.
diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go
@@ -45,12 +45,11 @@ type ArtifactCache map[string]ImageDetails
 // cache holds any data necessary for accessing the cache
 type cache struct {
 	artifactCache      ArtifactCache
-	dependencies       DependencyLister
 	client             docker.LocalDaemon
 	insecureRegistries map[string]bool
 	cacheFile          string
 	imagesAreLocal     bool
-	devMode            bool
+	hashForArtifact    func(ctx context.Context, a *latest.Artifact) (string, error)
 }
 
 // DependencyLister fetches a list of dependencies for an artifact
@@ -81,12 +80,13 @@ func NewCache(runCtx *runcontext.RunContext, imagesAreLocal bool, dependencies D
 
 	return &cache{
 		artifactCache:      artifactCache,
-		dependencies:       dependencies,
 		client:             client,
 		insecureRegistries: runCtx.InsecureRegistries,
 		cacheFile:          cacheFile,
 		imagesAreLocal:     imagesAreLocal,
-		devMode:            runCtx.DevMode,
+		hashForArtifact: func(ctx context.Context, a *latest.Artifact) (string, error) {
+			return getHashForArtifact(ctx, dependencies, a, runCtx.DevMode)
+		},
 	}, nil
 }
 

diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go
@@ -29,7 +29,6 @@ import (
 
 var (
 	// For testing
-	hashForArtifact = getHashForArtifact
 	buildInProgress = event.BuildInProgress
 )
 
@@ -53,7 +52,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, artifac
 }
 
 func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string) cacheDetails {
-	hash, err := hashForArtifact(ctx, c.dependencies, a, c.devMode)
+	hash, err := c.hashForArtifact(ctx, a)
 	if err != nil {
 		return failed{err: fmt.Errorf("getting hash for artifact %s: %v", a.ImageName, err)}
 	}

diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go
@@ -30,7 +30,7 @@ import (
 func TestLookupLocal(t *testing.T) {
 	tests := []struct {
 		description string
-		hasher      func(context.Context, DependencyLister, *latest.Artifact, bool) (string, error)
+		hasher      func(context.Context, *latest.Artifact) (string, error)
 		cache       map[string]ImageDetails
 		api         *testutil.FakeAPIClient
 		expected    cacheDetails
@@ -103,13 +103,13 @@ func TestLookupLocal(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			t.Override(&hashForArtifact, test.hasher)
 			t.Override(&buildInProgress, func(string) {})
 
 			cache := &cache{
-				imagesAreLocal: true,
-				artifactCache:  test.cache,
-				client:         docker.NewLocalDaemon(test.api, nil, false, nil),
+				imagesAreLocal:  true,
+				artifactCache:   test.cache,
+				client:          docker.NewLocalDaemon(test.api, nil, false, nil),
+				hashForArtifact: test.hasher,
 			}
 			details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": "tag"}, []*latest.Artifact{{
 				ImageName: "artifact",
@@ -126,7 +126,7 @@ func TestLookupLocal(t *testing.T) {
 func TestLookupRemote(t *testing.T) {
 	tests := []struct {
 		description string
-		hasher      func(context.Context, DependencyLister, *latest.Artifact, bool) (string, error)
+		hasher      func(context.Context, *latest.Artifact) (string, error)
 		cache       map[string]ImageDetails
 		api         *testutil.FakeAPIClient
 		expected    cacheDetails
@@ -178,7 +178,6 @@ func TestLookupRemote(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			t.Override(&hashForArtifact, test.hasher)
 			t.Override(&docker.RemoteDigest, func(identifier string, _ map[string]bool) (string, error) {
 				switch {
 				case identifier == "tag":
@@ -192,9 +191,10 @@ func TestLookupRemote(t *testing.T) {
 			t.Override(&buildInProgress, func(string) {})
 
 			cache := &cache{
-				imagesAreLocal: false,
-				artifactCache:  test.cache,
-				client:         docker.NewLocalDaemon(test.api, nil, false, nil),
+				imagesAreLocal:  false,
+				artifactCache:   test.cache,
+				client:          docker.NewLocalDaemon(test.api, nil, false, nil),
+				hashForArtifact: test.hasher,
 			}
 			details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": "tag"}, []*latest.Artifact{{
 				ImageName: "artifact",
@@ -208,14 +208,14 @@ func TestLookupRemote(t *testing.T) {
 	}
 }
 
-func mockHasher(value string) func(context.Context, DependencyLister, *latest.Artifact, bool) (string, error) {
-	return func(context.Context, DependencyLister, *latest.Artifact, bool) (string, error) {
+func mockHasher(value string) func(context.Context, *latest.Artifact) (string, error) {
+	return func(context.Context, *latest.Artifact) (string, error) {
 		return value, nil
 	}
 }
 
-func failingHasher(errMessage string) func(context.Context, DependencyLister, *latest.Artifact, bool) (string, error) {
-	return func(context.Context, DependencyLister, *latest.Artifact, bool) (string, error) {
+func failingHasher(errMessage string) func(context.Context, *latest.Artifact) (string, error) {
+	return func(context.Context, *latest.Artifact) (string, error) {
 		return "", errors.New(errMessage)
 	}
 }
diff --git a/pkg/skaffold/runner/runcontext/context.go b/pkg/skaffold/runner/runcontext/context.go
@@ -71,7 +71,6 @@ func GetRunContext(opts config.SkaffoldOptions, cfg latest.Pipeline) (*RunContex
 		insecureRegistries[r] = true
 	}
 
-	// TODO(dgageot): what about debug?
 	devMode := opts.Command == "dev"
 
 	return &RunContext{