Fix unnecessary warning in caching

pkg/skaffold/build/cache/cache.go

@@ -21,14 +21,14 @@ import (
 	"io/ioutil"
 	"path/filepath"
 
+	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	skafconfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
-	"github.com/docker/docker/api/types"
-
-	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
+	"github.com/docker/docker/api/types"
 	homedir "github.com/mitchellh/go-homedir"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -40,14 +40,15 @@ type ArtifactCache map[string]ImageDetails
 
 // Cache holds any data necessary for accessing the cache
 type Cache struct {
-	artifactCache ArtifactCache
-	client        docker.LocalDaemon
-	builder       build.Builder
-	imageList     []types.ImageSummary
-	cacheFile     string
-	useCache      bool
-	needsPush     bool
-	localCluster  bool
+	artifactCache  ArtifactCache
+	client         docker.LocalDaemon
+	builder        build.Builder
+	imageList      []types.ImageSummary
+	cacheFile      string
+	useCache       bool
+	isLocalBuilder bool
+	pushImages     bool
+	localCluster   bool
 }
 
 var (
@@ -59,7 +60,7 @@ var (
 )
 
 // NewCache returns the current state of the cache
-func NewCache(ctx context.Context, builder build.Builder, opts *skafconfig.SkaffoldOptions, needsPush bool) *Cache {
+func NewCache(builder build.Builder, opts *skafconfig.SkaffoldOptions, cfg latest.BuildConfig) *Cache {
 	if !opts.CacheArtifacts {
 		return noCache
 	}
@@ -79,7 +80,7 @@ func NewCache(ctx context.Context, builder build.Builder, opts *skafconfig.Skaff
 	}
 	var imageList []types.ImageSummary
 	if client != nil {
-		imageList, err = client.ImageList(ctx, types.ImageListOptions{})
+		imageList, err = client.ImageList(context.Background(), types.ImageListOptions{})
 		if err != nil {
 			logrus.Warn("Unable to get list of images from local docker daemon, won't be checked for cache.")
 		}
@@ -89,15 +90,17 @@ func NewCache(ctx context.Context, builder build.Builder, opts *skafconfig.Skaff
 	if err != nil {
 		logrus.Warn("Unable to determine if using a local cluster, cache may not work.")
 	}
+	pushImages := cfg.LocalBuild != nil && cfg.LocalBuild.Push != nil && *cfg.LocalBuild.Push
 	return &Cache{
-		artifactCache: cache,
-		cacheFile:     cf,
-		useCache:      opts.CacheArtifacts,
-		client:        client,
-		builder:       builder,
-		needsPush:     needsPush,
-		imageList:     imageList,
-		localCluster:  lc,
+		artifactCache:  cache,
+		cacheFile:      cf,
+		useCache:       opts.CacheArtifacts,
+		client:         client,
+		builder:        builder,
+		pushImages:     pushImages,
+		isLocalBuilder: cfg.LocalBuild != nil,
+		imageList:      imageList,
+		localCluster:   lc,
 	}
 }
 

pkg/skaffold/build/cache/cache_test.go

@@ -54,7 +54,7 @@ func mockHashForArtifact(hashes map[string]string) func(context.Context, build.B
 func Test_NewCache(t *testing.T) {
 	tests := []struct {
 		updateCacheFile   bool
-		needsPush         bool
+		pushImages        bool
 		updateClient      bool
 		name              string
 		opts              *config.SkaffoldOptions
@@ -85,22 +85,24 @@ func Test_NewCache(t *testing.T) {
 						ID: "image",
 					},
 				},
+				isLocalBuilder: true,
 			},
 		},
 		{
 			name:              "needs push",
 			cacheFileContents: defaultArtifactCache,
-			needsPush:         true,
+			pushImages:        true,
 			updateCacheFile:   true,
 			updateClient:      true,
 			opts: &config.SkaffoldOptions{
 				CacheArtifacts: true,
 			},
 			api: &testutil.FakeAPIClient{},
 			expectedCache: &Cache{
-				artifactCache: defaultArtifactCache,
-				useCache:      true,
-				needsPush:     true,
+				artifactCache:  defaultArtifactCache,
+				useCache:       true,
+				isLocalBuilder: true,
+				pushImages:     true,
 			},
 		},
 		{
@@ -144,7 +146,13 @@ func Test_NewCache(t *testing.T) {
 				test.expectedCache.client = docker.NewLocalDaemon(test.api, nil)
 			}
 
-			actualCache := NewCache(context.Background(), nil, test.opts, test.needsPush)
+			actualCache := NewCache(nil, test.opts, latest.BuildConfig{
+				BuildType: latest.BuildType{
+					LocalBuild: &latest.LocalBuild{
+						Push: &test.pushImages,
+					},
+				},
+			})
 
 			// cmp.Diff cannot access unexported fields, so use reflect.DeepEqual here directly
 			if !reflect.DeepEqual(test.expectedCache, actualCache) {

pkg/skaffold/build/cache/retrieve.go

@@ -148,7 +148,7 @@ func (c *Cache) retrieveCachedArtifactDetails(ctx context.Context, a *latest.Art
 	return &cachedArtifactDetails{
 		needsRebuild:  needsRebuild(il, c.localCluster),
 		needsRetag:    needsRetag(il),
-		needsPush:     needsPush(il, c.localCluster, c.needsPush),
+		needsPush:     needsPush(il, c.localCluster, c.pushImages),
 		prebuiltImage: il.prebuiltImage,
 		hashTag:       hashTag,
 	}, nil

pkg/skaffold/build/cache/retrieve_test.go

@@ -271,7 +271,7 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 			hashes:                    map[string]string{"image": "hash"},
 			cache: &Cache{
 				useCache:      true,
-				needsPush:     true,
+				pushImages:    true,
 				localCluster:  true,
 				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}},
 				imageList: []types.ImageSummary{
@@ -295,7 +295,7 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 			targetImageExistsRemotely: true,
 			cache: &Cache{
 				useCache:      true,
-				needsPush:     true,
+				pushImages:    true,
 				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}},
 			},
 			digest: digest,

pkg/skaffold/build/cache/save.go

@@ -33,10 +33,14 @@ import (
 )
 
 // Retag retags newly built images in the format [imageName:workspaceHash] and pushes them if using a remote cluster
-func (c *Cache) Retag(ctx context.Context, out io.Writer, artifactsToBuild []*latest.Artifact, buildArtifacts []build.Artifact) {
+func (c *Cache) RetagLocalImages(ctx context.Context, out io.Writer, artifactsToBuild []*latest.Artifact, buildArtifacts []build.Artifact) {
 	if !c.useCache || len(artifactsToBuild) == 0 {
 		return
 	}
+	// Built images remotely, nothing to retag
+	if !c.isLocalBuilder {
+		return
+	}
 	tags := map[string]string{}
 	for _, t := range buildArtifacts {
 		tags[t.ImageName] = t.Tag

pkg/skaffold/build/cache/save_test.go

@@ -0,0 +1,84 @@
+/*
+Copyright 2019 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cache
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/testutil"
+	"github.com/docker/docker/api/types"
+)
+
+func TestRetagLocalImages(t *testing.T) {
+	tests := []struct {
+		description      string
+		api              *testutil.FakeAPIClient
+		cache            *Cache
+		artifactsToBuild []*latest.Artifact
+		buildArtifacts   []build.Artifact
+		expectedPush     []string
+	}{
+		{
+			description: "retag and repush local image",
+			cache: &Cache{
+				useCache:       true,
+				isLocalBuilder: true,
+			},
+			api: &testutil.FakeAPIClient{
+				TagToImageID: map[string]string{"image:tag": "imageid"},
+				ImageSummaries: []types.ImageSummary{
+					{
+						ID:       "imageid",
+						RepoTags: []string{"image:tag"},
+					},
+				},
+			},
+			artifactsToBuild: []*latest.Artifact{
+				{
+					ImageName:     "image",
+					WorkspaceHash: "hash",
+				},
+			},
+			buildArtifacts: []build.Artifact{
+				{
+					ImageName: "image",
+					Tag:       "image:tag",
+				},
+			},
+			expectedPush: []string{"image:hash"},
+		}, {
+			description: "build images remotely",
+			api:         &testutil.FakeAPIClient{},
+			cache: &Cache{
+				useCache: true,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			test.cache.client = docker.NewLocalDaemon(test.api, nil)
+			test.cache.RetagLocalImages(context.Background(), os.Stdout, test.artifactsToBuild, test.buildArtifacts)
+			testutil.CheckErrorAndDeepEqual(t, false, nil, test.expectedPush, test.api.PushedImages)
+		})
+	}
+}

pkg/skaffold/runner/runner.go

@@ -57,11 +57,11 @@ type SkaffoldRunner struct {
 	sync.Syncer
 	watch.Watcher
 
+	cache             *cache.Cache
 	opts              *config.SkaffoldOptions
 	labellers         []deploy.Labeller
 	builds            []build.Artifact
 	hasDeployed       bool
-	needsPush         bool
 	imageList         *kubernetes.ImageList
 	namespaces        []string
 	RPCServerShutdown func() error
@@ -95,6 +95,8 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (*
 		return nil, errors.Wrap(err, "parsing build config")
 	}
 
+	artifactCache := cache.NewCache(builder, opts, cfg.Build)
+
 	tester, err := getTester(cfg.Test, opts)
 	if err != nil {
 		return nil, errors.Wrap(err, "parsing test config")
@@ -135,7 +137,7 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (*
 		labellers:         labellers,
 		imageList:         kubernetes.NewImageList(),
 		namespaces:        namespaces,
-		needsPush:         needsPush(cfg.Build),
+		cache:             artifactCache,
 		RPCServerShutdown: shutdown,
 	}, nil
 }
@@ -166,16 +168,6 @@ func getBuilder(cfg *latest.BuildConfig, kubeContext string, opts *config.Skaffo
 	}
 }
 
-func needsPush(cfg latest.BuildConfig) bool {
-	if cfg.LocalBuild == nil {
-		return false
-	}
-	if cfg.LocalBuild.Push == nil {
-		return false
-	}
-	return *cfg.LocalBuild.Push
-}
-
 func buildWithPlugin(artifacts []*latest.Artifact) bool {
 	for _, a := range artifacts {
 		if a.BuilderPlugin != nil {
@@ -349,8 +341,7 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 		return nil, errors.Wrap(err, "generating tag")
 	}
 
-	artifactCache := cache.NewCache(ctx, r.Builder, r.opts, r.needsPush)
-	artifactsToBuild, res, err := artifactCache.RetrieveCachedArtifacts(ctx, out, artifacts)
+	artifactsToBuild, res, err := r.cache.RetrieveCachedArtifacts(ctx, out, artifacts)
 	if err != nil {
 		return nil, errors.Wrap(err, "retrieving cached artifacts")
 	}
@@ -359,9 +350,9 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 	if err != nil {
 		return nil, errors.Wrap(err, "build failed")
 	}
-	artifactCache.Retag(ctx, out, artifactsToBuild, bRes)
+	r.cache.RetagLocalImages(ctx, out, artifactsToBuild, bRes)
 	bRes = append(bRes, res...)
-	if err := artifactCache.CacheArtifacts(ctx, artifacts, bRes); err != nil {
+	if err := r.cache.CacheArtifacts(ctx, artifacts, bRes); err != nil {
 		logrus.Warnf("error caching artifacts: %v", err)
 	}
 	if !r.opts.SkipTests {

testutil/fake_image_api.go

@@ -42,8 +42,9 @@ type FakeAPIClient struct {
 	ErrImagePull    bool
 	ErrStream       bool
 
-	nextImageID int
-	Pushed      []string
+	nextImageID  int
+	Pushed       []string
+	PushedImages []string
 }
 
 type errReader struct{}
@@ -123,6 +124,7 @@ func (f *FakeAPIClient) ImagePush(_ context.Context, ref string, _ types.ImagePu
 
 	digest := fmt.Sprintf("sha256:%x", sha256.New().Sum([]byte(f.TagToImageID[ref])))
 	f.Pushed = append(f.Pushed, digest)
+	f.PushedImages = append(f.PushedImages, ref)
 
 	return f.body(digest), nil
 }