From 360cbbdb2f9c29c705a103afe467e4dacec3afac Mon Sep 17 00:00:00 2001 From: David Gageot Date: Mon, 27 Jan 2020 13:23:54 +0100 Subject: [PATCH 1/2] Simplify hashForArtifact logic Also removed one of those global variables Signed-off-by: David Gageot --- pkg/skaffold/build/cache/cache.go | 8 +++---- pkg/skaffold/build/cache/lookup.go | 3 +-- pkg/skaffold/build/cache/lookup_test.go | 28 ++++++++++++------------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index f1d3178dafa..283781eb6a7 100644 --- 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 index afd891e2a16..5cbaa2b636f 100644 --- 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 index 52a943fc087..128d85e2bd7 100644 --- 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) } } From 217d00be2cfc2bdda1ae81ad00b52ced3924b381 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Mon, 27 Jan 2020 13:38:43 +0100 Subject: [PATCH 2/2] Remove TODO Signed-off-by: David Gageot --- pkg/skaffold/runner/runcontext/context.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/skaffold/runner/runcontext/context.go b/pkg/skaffold/runner/runcontext/context.go index d4232e6d9f5..a2b464f421b 100644 --- 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{