SyncMap is a matter of artifact type, not builder

Fixes #3448

pkg/skaffold/build/build.go

@@ -39,9 +39,6 @@ type Builder interface {
 
 	Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]Artifact, error)
 
-	// SyncMap provides a map of sync destinations by source paths.
-	SyncMap(ctx context.Context, artifact *latest.Artifact) (map[string][]string, error)
-
 	// Prune removes images built with Skaffold
 	Prune(context.Context, io.Writer) error
 }

pkg/skaffold/build/cluster/types.go

@@ -23,7 +23,6 @@ import (
 
 	"github.com/pkg/errors"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubectl"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
@@ -66,7 +65,3 @@ func (b *Builder) Labels() map[string]string {
 func (b *Builder) Prune(ctx context.Context, out io.Writer) error {
 	return nil
 }
-
-func (b *Builder) SyncMap(ctx context.Context, artifact *latest.Artifact) (map[string][]string, error) {
-	return nil, build.ErrSyncMapNotSupported{}
-}

pkg/skaffold/build/cluster/types_test.go

@@ -114,12 +114,6 @@ func TestPruneIsNoop(t *testing.T) {
 	testutil.CheckDeepEqual(t, nil, pruneError)
 }
 
-func TestSyncMapNotSupported(t *testing.T) {
-	syncMap, err := (&Builder{}).SyncMap(context.TODO(), nil)
-	var expected map[string][]string
-	testutil.CheckErrorAndDeepEqual(t, true, err, expected, syncMap)
-}
-
 func stubRunContext(clusterDetails *latest.ClusterDetails, insecureRegistries map[string]bool) *runcontext.RunContext {
 	pipeline := latest.Pipeline{}
 	pipeline.Build.BuildType.Cluster = clusterDetails

pkg/skaffold/build/gcb/types.go

@@ -23,7 +23,6 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/wait"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -102,7 +101,3 @@ func (b *Builder) Labels() map[string]string {
 func (b *Builder) Prune(ctx context.Context, out io.Writer) error {
 	return nil // noop
 }
-
-func (b *Builder) SyncMap(ctx context.Context, artifact *latest.Artifact) (map[string][]string, error) {
-	return nil, build.ErrSyncMapNotSupported{}
-}

pkg/skaffold/build/local/local.go

@@ -31,7 +31,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/jib"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 )
 
@@ -104,10 +103,3 @@ func (b *Builder) getImageIDForTag(ctx context.Context, tag string) (string, err
 	}
 	return insp.ID, nil
 }
-
-func (b *Builder) SyncMap(ctx context.Context, a *latest.Artifact) (map[string][]string, error) {
-	if a.DockerArtifact != nil {
-		return docker.SyncMap(ctx, a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, b.insecureRegistries)
-	}
-	return nil, build.ErrSyncMapNotSupported{}
-}

pkg/skaffold/docker/syncmap.go

@@ -17,7 +17,6 @@ limitations under the License.
 package docker
 
 import (
-	"context"
 	"os"
 	"path"
 	"path/filepath"
@@ -30,7 +29,7 @@ import (
 // SyncMap creates a map of syncable files by looking at the COPY/ADD commands in the Dockerfile.
 // All keys are relative to the Skaffold root, the destinations are absolute container paths.
 // TODO(corneliusweig) destinations are not resolved across stages in multistage dockerfiles. Is there a use-case for that?
-func SyncMap(ctx context.Context, workspace string, dockerfilePath string, buildArgs map[string]*string, insecureRegistries map[string]bool) (map[string][]string, error) {
+func SyncMap(workspace string, dockerfilePath string, buildArgs map[string]*string, insecureRegistries map[string]bool) (map[string][]string, error) {
 	absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
 	if err != nil {
 		return nil, errors.Wrap(err, "normalizing dockerfile path")

pkg/skaffold/docker/syncmap_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package docker
 
 import (
-	"context"
 	"path/filepath"
 	"sort"
 	"testing"
@@ -383,7 +382,7 @@ func TestSyncMap(t *testing.T) {
 			}
 
 			workspace := tmpDir.Path(test.workspace)
-			deps, err := SyncMap(context.Background(), workspace, "Dockerfile", test.buildArgs, nil)
+			deps, err := SyncMap(workspace, "Dockerfile", test.buildArgs, nil)
 
 			// destinations are not sorted, but for the test assertion they must be
 			for _, dsts := range deps {
@@ -480,7 +479,7 @@ ADD * .
 				Write("Dockerfile", test.dockerfile)
 
 			for i := 0; i < repeat; i++ {
-				deps, err := SyncMap(context.Background(), tmpDir.Root(), "Dockerfile", nil, nil)
+				deps, err := SyncMap(tmpDir.Root(), "Dockerfile", nil, nil)
 
 				// destinations are not sorted, but for the test assertion they must be
 				for _, dsts := range deps {

pkg/skaffold/runner/dev.go

@@ -145,8 +145,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 					return build.DependenciesForArtifact(ctx, artifact, r.runCtx.InsecureRegistries)
 				},
 				func(e filemon.Events) {
-					syncMap := func() (map[string][]string, error) { return r.builder.SyncMap(ctx, artifact) }
-					s, err := sync.NewItem(artifact, e, r.builds, r.runCtx.InsecureRegistries, syncMap)
+					s, err := sync.NewItem(artifact, e, r.builds, r.runCtx.InsecureRegistries)
 					switch {
 					case err != nil:
 						logrus.Warnf("error adding dirty artifact to changeset: %s", err.Error())

pkg/skaffold/runner/diagnose.go

@@ -30,6 +30,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync"
 )
 
 func (r *SkaffoldRunner) DiagnoseArtifacts(ctx context.Context, out io.Writer) error {
@@ -57,13 +58,13 @@ func (r *SkaffoldRunner) DiagnoseArtifacts(ctx context.Context, out io.Writer) e
 		fmt.Fprintln(out, " - Dependencies:", len(deps), "files")
 		fmt.Fprintf(out, " - Time to list dependencies: %v (2nd time: %v)\n", timeDeps1, timeDeps2)
 
-		timeSyncMap1, err := timeToConstructSyncMap(ctx, r.builder, artifact)
+		timeSyncMap1, err := timeToConstructSyncMap(artifact, r.runCtx.InsecureRegistries)
 		if err != nil {
 			if _, isNotSupported := err.(build.ErrSyncMapNotSupported); !isNotSupported {
 				return errors.Wrap(err, "construct artifact dependencies")
 			}
 		}
-		timeSyncMap2, err := timeToConstructSyncMap(ctx, r.builder, artifact)
+		timeSyncMap2, err := timeToConstructSyncMap(artifact, r.runCtx.InsecureRegistries)
 		if err != nil {
 			if _, isNotSupported := err.(build.ErrSyncMapNotSupported); !isNotSupported {
 				return errors.Wrap(err, "construct artifact dependencies")
@@ -112,9 +113,9 @@ func timeToListDependencies(ctx context.Context, a *latest.Artifact, insecureReg
 	return time.Since(start), paths, err
 }
 
-func timeToConstructSyncMap(ctx context.Context, builder build.Builder, a *latest.Artifact) (time.Duration, error) {
+func timeToConstructSyncMap(a *latest.Artifact, insecureRegistries map[string]bool) (time.Duration, error) {
 	start := time.Now()
-	_, err := builder.SyncMap(ctx, a)
+	_, err := sync.InferredSyncMap(a, insecureRegistries)
 	return time.Since(start), err
 }
 

pkg/skaffold/runner/runner_test.go

@@ -96,9 +96,6 @@ func (t *TestBench) TestDependencies() ([]string, error)              { return n
 func (t *TestBench) Dependencies() ([]string, error)                  { return nil, nil }
 func (t *TestBench) Cleanup(ctx context.Context, out io.Writer) error { return nil }
 func (t *TestBench) Prune(ctx context.Context, out io.Writer) error   { return nil }
-func (t *TestBench) SyncMap(ctx context.Context, artifact *latest.Artifact) (map[string][]string, error) {
-	return nil, nil
-}
 
 func (t *TestBench) enterNewCycle() {
 	t.actions = append(t.actions, t.currentActions)

pkg/skaffold/sync/sync.go

@@ -39,12 +39,13 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
+// For testing
 var (
-	// WorkingDir is here for testing
-	WorkingDir = docker.RetrieveWorkingDir
+	WorkingDir      = docker.RetrieveWorkingDir
+	InferredSyncMap = inferredSyncMap
 )
 
-func NewItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, insecureRegistries map[string]bool, destProvider DestinationProvider) (*Item, error) {
+func NewItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, insecureRegistries map[string]bool) (*Item, error) {
 	if !e.HasChanged() || a.Sync == nil {
 		return nil, nil
 	}
@@ -54,7 +55,7 @@ func NewItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, inse
 	}
 
 	if len(a.Sync.Infer) > 0 {
-		return inferredSyncItem(a, e, builds, destProvider)
+		return inferredSyncItem(a, e, builds, insecureRegistries)
 	}
 
 	return nil, nil
@@ -89,7 +90,7 @@ func manualSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artifac
 	return &Item{Image: tag, Copy: toCopy, Delete: toDelete}, nil
 }
 
-func inferredSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, provider DestinationProvider) (*Item, error) {
+func inferredSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, insecureRegistries map[string]bool) (*Item, error) {
 	// deleted files are no longer contained in the syncMap, so we need to rebuild
 	if len(e.Deleted) > 0 {
 		return nil, nil
@@ -100,7 +101,7 @@ func inferredSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artif
 		return nil, fmt.Errorf("could not find latest tag for image %s in builds: %v", a.ImageName, builds)
 	}
 
-	syncMap, err := provider()
+	syncMap, err := InferredSyncMap(a, insecureRegistries)
 	if err != nil {
 		return nil, errors.Wrapf(err, "inferring syncmap for image %s", a.ImageName)
 	}
@@ -138,6 +139,19 @@ func inferredSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artif
 	return &Item{Image: tag, Copy: toCopy}, nil
 }
 
+func inferredSyncMap(a *latest.Artifact, insecureRegistries map[string]bool) (map[string][]string, error) {
+	switch {
+	case a.DockerArtifact != nil:
+		return docker.SyncMap(a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, insecureRegistries)
+
+	case a.KanikoArtifact != nil:
+		return docker.SyncMap(a.Workspace, a.KanikoArtifact.DockerfilePath, a.KanikoArtifact.BuildArgs, insecureRegistries)
+
+	default:
+		return nil, build.ErrSyncMapNotSupported{}
+	}
+}
+
 func latestTag(image string, builds []build.Artifact) string {
 	for _, build := range builds {
 		if build.ImageName == image {

pkg/skaffold/sync/sync_test.go

@@ -637,12 +637,10 @@ func TestNewSyncItem(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			t.Override(&WorkingDir, func(string, map[string]bool) (string, error) {
-				return test.workingDir, nil
-			})
+			t.Override(&WorkingDir, func(string, map[string]bool) (string, error) { return test.workingDir, nil })
+			t.Override(&InferredSyncMap, func(*latest.Artifact, map[string]bool) (map[string][]string, error) { return test.dependencies, nil })
 
-			provider := func() (map[string][]string, error) { return test.dependencies, nil }
-			actual, err := NewItem(test.artifact, test.evt, test.builds, nil, provider)
+			actual, err := NewItem(test.artifact, test.evt, test.builds, nil)
 
 			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, actual)
 		})
@@ -853,3 +851,55 @@ func TestPerform(t *testing.T) {
 		})
 	}
 }
+
+func TestInferredSyncMap(t *testing.T) {
+	tests := []struct {
+		description  string
+		artifactType latest.ArtifactType
+		files        map[string]string
+		shouldErr    bool
+		expectedMap  map[string][]string
+	}{
+		{
+			description: "docker - supported",
+			artifactType: latest.ArtifactType{
+				DockerArtifact: &latest.DockerArtifact{
+					DockerfilePath: "Dockerfile",
+				},
+			},
+			files: map[string]string{
+				"Dockerfile": "FROM alpine\nCOPY *.go /app/",
+				"main.go":    "",
+			},
+			expectedMap: map[string][]string{"main.go": {"/app/main.go"}},
+		},
+		{
+			description: "kaniko - supported",
+			artifactType: latest.ArtifactType{
+				KanikoArtifact: &latest.KanikoArtifact{
+					DockerfilePath: "Dockerfile",
+				},
+			},
+			files: map[string]string{
+				"Dockerfile": "FROM alpine\nCOPY *.go /app/",
+				"main.go":    "",
+			},
+			expectedMap: map[string][]string{"main.go": {"/app/main.go"}},
+		},
+		{
+			description:  "not supported",
+			artifactType: latest.ArtifactType{},
+			shouldErr:    true,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.NewTempDir().WriteFiles(test.files).Chdir()
+
+			syncMap, err := InferredSyncMap(&latest.Artifact{ArtifactType: test.artifactType}, nil)
+
+			t.CheckError(test.shouldErr, err)
+			t.CheckDeepEqual(test.expectedMap, syncMap)
+		})
+	}
+}

pkg/skaffold/sync/types.go

@@ -32,8 +32,6 @@ type Item struct {
 	Delete map[string][]string
 }
 
-type DestinationProvider func() (map[string][]string, error)
-
 type Syncer interface {
 	Sync(context.Context, *Item) error
 }