Refactor code to simplify GoogleContainerTools#3395

Signed-off-by: David Gageot <[email protected]>

Author: dgageot

pkg/skaffold/build/buildpacks/build.go

@@ -27,8 +27,8 @@ import (
 
 // Build builds an artifact with Cloud Native Buildpacks:
 // https://buildpacks.io/
-func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) {
-	built, err := b.build(ctx, out, a.Workspace, a.BuildpackArtifact, tag)
+func (b *Builder) Build(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) {
+	built, err := b.build(ctx, out, artifact, tag)
 	if err != nil {
 		return "", err
 	}

pkg/skaffold/build/buildpacks/lifecycle.go

@@ -33,7 +33,10 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
-func (b *Builder) build(ctx context.Context, out io.Writer, workspace string, artifact *latest.BuildpackArtifact, tag string) (string, error) {
+func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) {
+	artifact := a.BuildpackArtifact
+	workspace := a.Workspace
+
 	// To improve caching, we always build the image with [:latest] tag
 	// This way, the lifecycle is able to "bootstrap" from the previously built image.
 	// The image will then be tagged as usual with the tag provided by the tag policy.

pkg/skaffold/docker/image_util.go

@@ -27,30 +27,40 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 )
 
-func RetrieveWorkingDir(tagged string, insecureRegistries map[string]bool) (string, error) {
-	var cf *v1.ConfigFile
-	var err error
-
+func RetrieveConfigFile(tagged string, runCtx *runcontext.RunContext) (*v1.ConfigFile, error) {
 	if strings.ToLower(tagged) == "scratch" {
-		return "/", nil
+		return nil, nil
 	}
 
-	// TODO: use the proper RunContext
-	localDocker, err := NewAPIClient(&runcontext.RunContext{})
+	var cf *v1.ConfigFile
+	var err error
+
+	localDocker, err := NewAPIClient(runCtx)
 	if err == nil {
 		cf, err = localDocker.ConfigFile(context.Background(), tagged)
 	}
 	if err != nil {
 		// No local Docker is available
-		cf, err = RetrieveRemoteConfig(tagged, insecureRegistries)
+		cf, err = RetrieveRemoteConfig(tagged, runCtx.InsecureRegistries)
 	}
 	if err != nil {
-		return "", errors.Wrap(err, "retrieving image config")
+		return nil, errors.Wrap(err, "retrieving image config")
 	}
 
-	if cf.Config.WorkingDir == "" {
+	return cf, err
+}
+
+func RetrieveWorkingDir(tagged string, runCtx *runcontext.RunContext) (string, error) {
+	cf, err := RetrieveConfigFile(tagged, runCtx)
+	switch {
+	case err != nil:
+		return "", err
+	case cf == nil:
+		return "/", nil
+	case cf.Config.WorkingDir == "":
 		logrus.Debugf("Using default workdir '/' for %s", tagged)
 		return "/", nil
+	default:
+		return cf.Config.WorkingDir, nil
 	}
-	return cf.Config.WorkingDir, nil
 }

pkg/skaffold/runner/dev.go

@@ -146,7 +146,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 				},
 				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, syncMap)
 					switch {
 					case err != nil:
 						logrus.Warnf("error adding dirty artifact to changeset: %s", err.Error())

pkg/skaffold/runner/dev_test.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/client-go/tools/clientcmd/api"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync"
 	"github.com/GoogleContainerTools/skaffold/testutil"
@@ -342,11 +343,11 @@ func TestDevSync(t *testing.T) {
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			var actualFileSyncEventCalls fileSyncEventCalls
+			t.SetupFakeKubernetesContext(api.Config{CurrentContext: "cluster1"})
 			t.Override(&fileSyncInProgress, func(int, string) { actualFileSyncEventCalls.InProgress++ })
 			t.Override(&fileSyncFailed, func(int, string, error) { actualFileSyncEventCalls.Failed++ })
 			t.Override(&fileSyncSucceeded, func(int, string) { actualFileSyncEventCalls.Succeeded++ })
-			t.SetupFakeKubernetesContext(api.Config{CurrentContext: "cluster1"})
-			t.Override(&sync.WorkingDir, func(string, map[string]bool) (string, error) { return "/", nil })
+			t.Override(&sync.WorkingDir, func(string, *runcontext.RunContext) (string, error) { return "/", nil })
 			test.testBench.cycles = len(test.watchEvents)
 
 			runner := createRunner(t, test.testBench, &TestMonitor{

pkg/skaffold/sync/sync.go

@@ -35,6 +35,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
@@ -44,29 +45,29 @@ var (
 	WorkingDir = docker.RetrieveWorkingDir
 )
 
-func NewItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, insecureRegistries map[string]bool, destProvider DestinationProvider) (*Item, error) {
-	if !e.HasChanged() || a.Sync == nil {
+func NewItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, runCtx *runcontext.RunContext, destProvider DestinationProvider) (*Item, error) {
+	switch {
+	case !e.HasChanged():
 		return nil, nil
-	}
 
-	if len(a.Sync.Manual) > 0 {
-		return manualSyncItem(a, e, builds, insecureRegistries)
-	}
+	case a.Sync != nil && len(a.Sync.Manual) > 0:
+		return manualSyncItem(a, e, builds, runCtx)
 
-	if len(a.Sync.Infer) > 0 {
+	case a.Sync != nil && len(a.Sync.Infer) > 0:
 		return inferredSyncItem(a, e, builds, destProvider)
-	}
 
-	return nil, nil
+	default:
+		return nil, nil
+	}
 }
 
-func manualSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, insecureRegistries map[string]bool) (*Item, error) {
+func manualSyncItem(a *latest.Artifact, e filemon.Events, builds []build.Artifact, runCtx *runcontext.RunContext) (*Item, error) {
 	tag := latestTag(a.ImageName, builds)
 	if tag == "" {
 		return nil, fmt.Errorf("could not find latest tag for image %s in builds: %v", a.ImageName, builds)
 	}
 
-	containerWd, err := WorkingDir(tag, insecureRegistries)
+	containerWd, err := WorkingDir(tag, runCtx)
 	if err != nil {
 		return nil, errors.Wrapf(err, "retrieving working dir for %s", tag)
 	}

pkg/skaffold/sync/sync_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
 	pkgkubernetes "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
@@ -637,12 +638,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, *runcontext.RunContext) (string, error) { return test.workingDir, 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, &runcontext.RunContext{}, provider)
 
 			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, actual)
 		})