Merge pull request #540 from dgageot/simpler-builder-deployer

Simpler runner code

pkg/skaffold/build/build.go

@@ -24,11 +24,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
 )
 
-// BuildResult holds the results of builds
-type BuildResult struct {
-	Builds []Build
-}
-
 // Build is the result corresponding to each Artifact built.
 type Build struct {
 	ImageName string
@@ -41,5 +36,5 @@ type Build struct {
 // This could include pushing to a authorized repository or loading the nodes with the image.
 // If artifacts is supplied, the builder should only rebuild those artifacts.
 type Builder interface {
-	Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) (*BuildResult, error)
+	Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error)
 }

pkg/skaffold/build/container_builder.go

@@ -77,22 +77,25 @@ func NewGoogleCloudBuilder(cfg *v1alpha2.BuildConfig) (*GoogleCloudBuilder, erro
 	return &GoogleCloudBuilder{cfg}, nil
 }
 
-func (cb *GoogleCloudBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) (*BuildResult, error) {
+func (cb *GoogleCloudBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) {
 	client, err := google.DefaultClient(ctx, cloudbuild.CloudPlatformScope)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting google client")
 	}
+
 	cbclient, err := cloudbuild.New(client)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting builder")
 	}
+
 	cbclient.UserAgent = version.UserAgent()
 	c, err := cstorage.NewClient(ctx)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting cloud storage client")
 	}
 	defer c.Close()
-	builds := []Build{}
+
+	var builds []Build
 	for _, artifact := range artifacts {
 		build, err := cb.buildArtifact(ctx, out, tagger, cbclient, c, artifact)
 		if err != nil {
@@ -101,9 +104,7 @@ func (cb *GoogleCloudBuilder) Build(ctx context.Context, out io.Writer, tagger t
 		builds = append(builds, *build)
 	}
 
-	return &BuildResult{
-		Builds: builds,
-	}, nil
+	return builds, nil
 }
 
 func (cb *GoogleCloudBuilder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.Tagger, cbclient *cloudbuild.Service, c *cstorage.Client, artifact *v1alpha2.Artifact) (*Build, error) {

pkg/skaffold/build/deps.go

@@ -29,6 +29,10 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+// DependencyMapFactory can build DependencyMaps from a list of artifacts.
+type DependencyMapFactory func(artifacts []*v1alpha2.Artifact) (*DependencyMap, error)
+
+// DependencyMap is a bijection between artifacts and the files they depend on.
 type DependencyMap struct {
 	artifacts       []*v1alpha2.Artifact
 	pathToArtifacts map[string][]*v1alpha2.Artifact
@@ -70,10 +74,15 @@ func NewDependencyMap(artifacts []*v1alpha2.Artifact) (*DependencyMap, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "generating path to artifact map")
 	}
+
+	return NewExplicitDependencyMap(artifacts, m), nil
+}
+
+func NewExplicitDependencyMap(artifacts []*v1alpha2.Artifact, pathToArtifacts map[string][]*v1alpha2.Artifact) *DependencyMap {
 	return &DependencyMap{
 		artifacts:       artifacts,
-		pathToArtifacts: m,
-	}, nil
+		pathToArtifacts: pathToArtifacts,
+	}
 }
 
 func isIgnored(path string) (bool, error) {

pkg/skaffold/build/kaniko.go

@@ -42,9 +42,7 @@ func NewKanikoBuilder(cfg *v1alpha2.BuildConfig) (*KanikoBuilder, error) {
 	}, nil
 }
 
-func (k *KanikoBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) (*BuildResult, error) {
-	res := &BuildResult{}
-
+func (k *KanikoBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) {
 	client, err := kubernetes.GetClientset()
 	if err != nil {
 		return nil, errors.Wrap(err, "getting kubernetes client")
@@ -74,6 +72,8 @@ func (k *KanikoBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tag
 	}()
 
 	// TODO(r2d4): parallel builds
+	var builds []Build
+
 	for _, artifact := range artifacts {
 		initialTag, err := kaniko.RunKanikoBuild(ctx, out, artifact, k.KanikoBuild)
 		if err != nil {
@@ -97,11 +97,12 @@ func (k *KanikoBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tag
 			return nil, errors.Wrap(err, "tagging image")
 		}
 
-		res.Builds = append(res.Builds, Build{
+		builds = append(builds, Build{
 			ImageName: artifact.ImageName,
 			Tag:       tag,
 			Artifact:  artifact,
 		})
 	}
-	return res, nil
+
+	return builds, nil
 }

pkg/skaffold/build/local.go

@@ -77,15 +77,16 @@ func (l *LocalBuilder) runBuildForArtifact(ctx context.Context, out io.Writer, a
 
 // Build runs a docker build on the host and tags the resulting image with
 // its checksum. It streams build progress to the writer argument.
-func (l *LocalBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) (*BuildResult, error) {
+func (l *LocalBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*v1alpha2.Artifact) ([]Build, error) {
 	if l.localCluster {
 		if _, err := fmt.Fprintf(out, "Found [%s] context, using local docker daemon.\n", l.kubeContext); err != nil {
 			return nil, errors.Wrap(err, "writing status")
 		}
 	}
 	defer l.api.Close()
 
-	res := &BuildResult{}
+	var builds []Build
+
 	for _, artifact := range artifacts {
 		initialTag, err := l.runBuildForArtifact(ctx, out, artifact)
 		if err != nil {
@@ -118,14 +119,14 @@ func (l *LocalBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagg
 			}
 		}
 
-		res.Builds = append(res.Builds, Build{
+		builds = append(builds, Build{
 			ImageName: artifact.ImageName,
 			Tag:       tag,
 			Artifact:  artifact,
 		})
 	}
 
-	return res, nil
+	return builds, nil
 }
 
 func (l *LocalBuilder) buildDocker(ctx context.Context, out io.Writer, a *v1alpha2.Artifact) (string, error) {

pkg/skaffold/build/local_test.go

@@ -79,9 +79,8 @@ func TestLocalRun(t *testing.T) {
 		tagger       tag.Tagger
 		localCluster bool
 		artifacts    []*v1alpha2.Artifact
-
-		expectedBuild *BuildResult
-		shouldErr     bool
+		expected     []Build
+		shouldErr    bool
 	}{
 		{
 			description: "single build",
@@ -98,13 +97,11 @@ func TestLocalRun(t *testing.T) {
 			},
 			tagger: &tag.ChecksumTagger{},
 			api:    testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{}),
-			expectedBuild: &BuildResult{
-				[]Build{
-					{
-						ImageName: "gcr.io/test/image",
-						Tag:       "gcr.io/test/image:imageid",
-						Artifact:  testImage1,
-					},
+			expected: []Build{
+				{
+					ImageName: "gcr.io/test/image",
+					Tag:       "gcr.io/test/image:imageid",
+					Artifact:  testImage1,
 				},
 			},
 		},
@@ -145,13 +142,11 @@ func TestLocalRun(t *testing.T) {
 				},
 			},
 			api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{}),
-			expectedBuild: &BuildResult{
-				[]Build{
-					{
-						ImageName: "gcr.io/test/image",
-						Tag:       "gcr.io/test/image:imageid",
-						Artifact:  testImage1,
-					},
+			expected: []Build{
+				{
+					ImageName: "gcr.io/test/image",
+					Tag:       "gcr.io/test/image:imageid",
+					Artifact:  testImage1,
 				},
 			},
 		},
@@ -254,8 +249,9 @@ func TestLocalRun(t *testing.T) {
 			if test.artifacts == nil {
 				test.artifacts = test.config.Artifacts
 			}
+
 			res, err := l.Build(context.Background(), test.out, test.tagger, test.artifacts)
-			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expectedBuild, res)
+			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, res)
 		})
 	}
 }

pkg/skaffold/constants/constants.go

@@ -39,7 +39,4 @@ const (
 	GCSBucketSuffix                = "_cloudbuild"
 
 	DefaultKanikoImage = "gcr.io/kaniko-project/executor:latest"
-
-	// TerminalBell is the sequence that triggers a beep in the terminal
-	TerminalBell = "\007"
 )

pkg/skaffold/deploy/deploy.go

@@ -24,16 +24,12 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 )
 
-// Result is currently unused, but a stub for results that might be returned
-// from a Deployer.Run()
-type Result struct{}
-
 // Deployer is the Deploy API of skaffold and responsible for deploying
 // the build results to a Kubernetes cluster
 type Deployer interface {
 	// Deploy should ensure that the build results are deployed to the Kubernetes
 	// cluster.
-	Deploy(context.Context, io.Writer, *build.BuildResult) (*Result, error)
+	Deploy(context.Context, io.Writer, []build.Build) error
 
 	// Dependencies returns a list of files that the deployer depends on.
 	// In dev mode, a redeploy will be triggered
@@ -43,9 +39,9 @@ type Deployer interface {
 	Cleanup(context.Context, io.Writer) error
 }
 
-func JoinTagsToBuildResult(b []build.Build, params map[string]string) (map[string]build.Build, error) {
+func JoinTagsToBuildResult(builds []build.Build, params map[string]string) (map[string]build.Build, error) {
 	imageToBuildResult := map[string]build.Build{}
-	for _, build := range b {
+	for _, build := range builds {
 		imageToBuildResult[build.ImageName] = build
 	}
 

pkg/skaffold/deploy/helm.go

@@ -43,13 +43,13 @@ func NewHelmDeployer(cfg *v1alpha2.DeployConfig, kubeContext string) *HelmDeploy
 	}
 }
 
-func (h *HelmDeployer) Deploy(ctx context.Context, out io.Writer, b *build.BuildResult) (*Result, error) {
+func (h *HelmDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Build) error {
 	for _, r := range h.HelmDeploy.Releases {
-		if err := h.deployRelease(out, r, b); err != nil {
-			return nil, errors.Wrapf(err, "deploying %s", r.Name)
+		if err := h.deployRelease(out, r, builds); err != nil {
+			return errors.Wrapf(err, "deploying %s", r.Name)
 		}
 	}
-	return nil, nil
+	return nil
 }
 
 // Not implemented
@@ -77,14 +77,13 @@ func (h *HelmDeployer) helm(out io.Writer, arg ...string) error {
 	return util.RunCmd(cmd)
 }
 
-func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, b *build.BuildResult) error {
+func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, builds []build.Build) error {
 	isInstalled := true
 	if err := h.helm(out, "get", r.Name); err != nil {
 		fmt.Fprintf(out, "Helm release %s not installed. Installing...\n", r.Name)
 		isInstalled = false
 	}
-
-	params, err := JoinTagsToBuildResult(b.Builds, r.Values)
+	params, err := JoinTagsToBuildResult(builds, r.Values)
 	if err != nil {
 		return errors.Wrap(err, "matching build results to chart values")
 	}

pkg/skaffold/deploy/helm_test.go

@@ -29,12 +29,10 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
-var testBuildResult = &build.BuildResult{
-	Builds: []build.Build{
-		{
-			ImageName: "skaffold-helm",
-			Tag:       "skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184",
-		},
+var testBuilds = []build.Build{
+	{
+		ImageName: "skaffold-helm",
+		Tag:       "skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184",
 	},
 }
 
@@ -78,21 +76,20 @@ func TestHelmDeploy(t *testing.T) {
 		description string
 		cmd         util.Command
 		deployer    *HelmDeployer
-		buildResult *build.BuildResult
-
-		shouldErr bool
+		builds      []build.Build
+		shouldErr   bool
 	}{
 		{
 			description: "deploy success",
 			cmd:         &MockHelm{t: t},
 			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
-			buildResult: testBuildResult,
+			builds:      testBuilds,
 		},
 		{
 			description: "deploy error unmatched parameter",
 			cmd:         &MockHelm{t: t},
 			deployer:    NewHelmDeployer(testDeployConfigParameterUnmatched, testKubeContext),
-			buildResult: testBuildResult,
+			builds:      testBuilds,
 			shouldErr:   true,
 		},
 		{
@@ -102,37 +99,37 @@ func TestHelmDeploy(t *testing.T) {
 				getResult:     cmdOutput{"", fmt.Errorf("not found")},
 				upgradeResult: cmdOutput{"", fmt.Errorf("should not have called upgrade")},
 			},
-			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
-			buildResult: testBuildResult,
+			deployer: NewHelmDeployer(testDeployConfig, testKubeContext),
+			builds:   testBuilds,
 		},
 		{
 			description: "get success should upgrade not install",
 			cmd: &MockHelm{
 				t:             t,
 				installResult: cmdOutput{"", fmt.Errorf("should not have called install")},
 			},
-			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
-			buildResult: testBuildResult,
+			deployer: NewHelmDeployer(testDeployConfig, testKubeContext),
+			builds:   testBuilds,
 		},
 		{
 			description: "deploy error",
 			cmd: &MockHelm{
 				t:             t,
 				upgradeResult: cmdOutput{"", fmt.Errorf("unexpected error")},
 			},
-			shouldErr:   true,
-			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
-			buildResult: testBuildResult,
+			shouldErr: true,
+			deployer:  NewHelmDeployer(testDeployConfig, testKubeContext),
+			builds:    testBuilds,
 		},
 		{
 			description: "dep build error",
 			cmd: &MockHelm{
 				t:         t,
 				depResult: cmdOutput{"", fmt.Errorf("unexpected error")},
 			},
-			shouldErr:   true,
-			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
-			buildResult: testBuildResult,
+			shouldErr: true,
+			deployer:  NewHelmDeployer(testDeployConfig, testKubeContext),
+			builds:    testBuilds,
 		},
 	}
 
@@ -141,11 +138,10 @@ func TestHelmDeploy(t *testing.T) {
 			defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand)
 			util.DefaultExecCommand = tt.cmd
 
-			_, err := tt.deployer.Deploy(context.Background(), &bytes.Buffer{}, tt.buildResult)
+			err := tt.deployer.Deploy(context.Background(), &bytes.Buffer{}, tt.builds)
 			testutil.CheckError(t, tt.shouldErr, err)
 		})
 	}
-
 }
 
 type MockHelm struct {