Embed labelling into Deployers

This makes it easier to implement <> labelling
strategies.

pkg/skaffold/deploy/deploy.go

@@ -21,7 +21,6 @@ import (
 	"io"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
-
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -38,7 +37,7 @@ type Deployer interface {
 
 	// Deploy should ensure that the build results are deployed to the Kubernetes
 	// cluster.
-	Deploy(context.Context, io.Writer, []build.Artifact) ([]Artifact, error)
+	Deploy(context.Context, io.Writer, []build.Artifact, []Labeller) error
 
 	// Dependencies returns a list of files that the deployer depends on.
 	// In dev mode, a redeploy will be triggered

pkg/skaffold/deploy/helm.go

@@ -66,17 +66,23 @@ func (h *HelmDeployer) Labels() map[string]string {
 	}
 }
 
-func (h *HelmDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]Artifact, error) {
-	deployResults := []Artifact{}
+func (h *HelmDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact, labellers []Labeller) error {
+	var dRes []Artifact
+
+	labels := merge(labellers...)
+
 	for _, r := range h.Releases {
 		results, err := h.deployRelease(ctx, out, r, builds)
 		if err != nil {
 			releaseName, _ := evaluateReleaseName(r.Name)
-			return deployResults, errors.Wrapf(err, "deploying %s", releaseName)
+			return errors.Wrapf(err, "deploying %s", releaseName)
 		}
-		deployResults = append(deployResults, results...)
+
+		dRes = append(dRes, results...)
 	}
-	return deployResults, nil
+
+	labelDeployResults(labels, dRes)
+	return nil
 }
 
 func (h *HelmDeployer) Dependencies() ([]string, error) {

pkg/skaffold/deploy/helm_test.go

@@ -383,7 +383,7 @@ 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(), ioutil.Discard, tt.builds)
+			err := tt.deployer.Deploy(context.Background(), ioutil.Discard, tt.builds, nil)
 
 			testutil.CheckError(t, tt.shouldErr, err)
 		})

pkg/skaffold/deploy/kubectl.go

@@ -66,32 +66,36 @@ func (k *KubectlDeployer) Labels() map[string]string {
 
 // Deploy templates the provided manifests with a simple `find and replace` and
 // runs `kubectl apply` on those manifests
-func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]Artifact, error) {
+func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact, labellers []Labeller) error {
 	color.Default.Fprintln(out, "kubectl client version:", k.kubectl.Version())
 	if err := k.kubectl.CheckVersion(); err != nil {
 		color.Default.Fprintln(out, err)
 	}
 
 	manifests, err := k.readManifests(ctx)
 	if err != nil {
-		return nil, errors.Wrap(err, "reading manifests")
+		return errors.Wrap(err, "reading manifests")
 	}
 
 	if len(manifests) == 0 {
-		return nil, nil
+		return nil
 	}
 
 	manifests, err = manifests.ReplaceImages(builds, k.defaultRepo)
 	if err != nil {
-		return nil, errors.Wrap(err, "replacing images in manifests")
+		return errors.Wrap(err, "replacing images in manifests")
 	}
 
 	updated, err := k.kubectl.Apply(ctx, out, manifests)
 	if err != nil {
-		return nil, errors.Wrap(err, "apply")
+		return errors.Wrap(err, "apply")
 	}
 
-	return parseManifestsForDeploys(k.kubectl.Namespace, updated)
+	dRes := parseManifestsForDeploys(k.kubectl.Namespace, updated)
+	labels := merge(labellers...)
+	labelDeployResults(labels, dRes)
+
+	return nil
 }
 
 // Cleanup deletes what was deployed by calling Deploy.
@@ -133,15 +137,15 @@ func (k *KubectlDeployer) manifestFiles(manifests []string) ([]string, error) {
 	return filteredManifests, nil
 }
 
-func parseManifestsForDeploys(namespace string, manifests kubectl.ManifestList) ([]Artifact, error) {
+func parseManifestsForDeploys(namespace string, manifests kubectl.ManifestList) []Artifact {
 	var results []Artifact
 
 	for _, manifest := range manifests {
 		b := bufio.NewReader(bytes.NewReader(manifest))
 		results = append(results, parseReleaseInfo(namespace, b)...)
 	}
 
-	return results, nil
+	return results
 }
 
 // readManifests reads the manifests to deploy/delete.

pkg/skaffold/deploy/kubectl_test.go

@@ -150,7 +150,7 @@ func TestKubectlDeploy(t *testing.T) {
 			}
 
 			k := NewKubectlDeployer(tmpDir.Root(), test.cfg, testKubeContext, testNamespace, "")
-			_, err := k.Deploy(context.Background(), ioutil.Discard, test.builds)
+			err := k.Deploy(context.Background(), ioutil.Discard, test.builds, nil)
 
 			testutil.CheckError(t, test.shouldErr, err)
 		})
@@ -254,23 +254,23 @@ spec:
 	deployer := NewKubectlDeployer(tmpDir.Root(), cfg, testKubeContext, testNamespace, "")
 
 	// Deploy one manifest
-	_, err := deployer.Deploy(context.Background(), ioutil.Discard, []build.Artifact{
+	err := deployer.Deploy(context.Background(), ioutil.Discard, []build.Artifact{
 		{ImageName: "leeroy-web", Tag: "leeroy-web:v1"},
 		{ImageName: "leeroy-app", Tag: "leeroy-app:v1"},
-	})
+	}, nil)
 	testutil.CheckError(t, false, err)
 
 	// Deploy one manifest since only one image is updated
-	_, err = deployer.Deploy(context.Background(), ioutil.Discard, []build.Artifact{
+	err = deployer.Deploy(context.Background(), ioutil.Discard, []build.Artifact{
 		{ImageName: "leeroy-web", Tag: "leeroy-web:v1"},
 		{ImageName: "leeroy-app", Tag: "leeroy-app:v2"},
-	})
+	}, nil)
 	testutil.CheckError(t, false, err)
 
 	// Deploy zero manifest since no image is updated
-	_, err = deployer.Deploy(context.Background(), ioutil.Discard, []build.Artifact{
+	err = deployer.Deploy(context.Background(), ioutil.Discard, []build.Artifact{
 		{ImageName: "leeroy-web", Tag: "leeroy-web:v1"},
 		{ImageName: "leeroy-app", Tag: "leeroy-app:v2"},
-	})
+	}, nil)
 	testutil.CheckError(t, false, err)
 }

pkg/skaffold/deploy/kustomize.go

@@ -80,32 +80,36 @@ func (k *KustomizeDeployer) Labels() map[string]string {
 }
 
 // Deploy runs `kubectl apply` on the manifest generated by kustomize.
-func (k *KustomizeDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]Artifact, error) {
+func (k *KustomizeDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact, labellers []Labeller) error {
 	color.Default.Fprintln(out, "kubectl client version:", k.kubectl.Version())
 	if err := k.kubectl.CheckVersion(); err != nil {
 		color.Default.Fprintln(out, err)
 	}
 
 	manifests, err := k.readManifests(ctx)
 	if err != nil {
-		return nil, errors.Wrap(err, "reading manifests")
+		return errors.Wrap(err, "reading manifests")
 	}
 
 	if len(manifests) == 0 {
-		return nil, nil
+		return nil
 	}
 
 	manifests, err = manifests.ReplaceImages(builds, k.defaultRepo)
 	if err != nil {
-		return nil, errors.Wrap(err, "replacing images in manifests")
+		return errors.Wrap(err, "replacing images in manifests")
 	}
 
 	updated, err := k.kubectl.Apply(ctx, out, manifests)
 	if err != nil {
-		return nil, errors.Wrap(err, "apply")
+		return errors.Wrap(err, "apply")
 	}
 
-	return parseManifestsForDeploys(k.kubectl.Namespace, updated)
+	dRes := parseManifestsForDeploys(k.kubectl.Namespace, updated)
+	labels := merge(labellers...)
+	labelDeployResults(labels, dRes)
+
+	return nil
 }
 
 // Cleanup deletes what was deployed by calling Deploy.

pkg/skaffold/deploy/labels.go

@@ -17,26 +17,21 @@ limitations under the License.
 package deploy
 
 import (
-	"context"
 	"encoding/json"
 	"fmt"
-	"io"
 	"strings"
 	"time"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
 	kubectx "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/context"
-
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	patch "k8s.io/apimachinery/pkg/util/strategicpatch"
-
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/dynamic"
 )
@@ -46,28 +41,6 @@ type Labeller interface {
 	Labels() map[string]string
 }
 
-type withLabels struct {
-	Deployer
-
-	labellers []Labeller
-}
-
-// WithLabels creates a deployer that sets labels on deployed resources.
-func WithLabels(d Deployer, labellers ...Labeller) Deployer {
-	return &withLabels{
-		Deployer:  d,
-		labellers: labellers,
-	}
-}
-
-func (w *withLabels) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) ([]Artifact, error) {
-	dRes, err := w.Deployer.Deploy(ctx, out, artifacts)
-
-	labelDeployResults(merge(w.labellers...), dRes)
-
-	return dRes, err
-}
-
 // merge merges the labels from multiple sources.
 func merge(sources ...Labeller) map[string]string {
 	merged := make(map[string]string)

pkg/skaffold/runner/dev.go

@@ -79,7 +79,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 				return nil
 			}
 		case changed.needsRedeploy:
-			if _, err := r.Deploy(ctx, out, r.builds); err != nil {
+			if err := r.Deploy(ctx, out, r.builds); err != nil {
 				logrus.Warnln("Skipping deploy due to error:", err)
 				return nil
 			}

pkg/skaffold/runner/notification.go

@@ -41,13 +41,12 @@ type withNotification struct {
 	deploy.Deployer
 }
 
-func (w withNotification) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]deploy.Artifact, error) {
-	res, err := w.Deployer.Deploy(ctx, out, builds)
-	if err != nil {
-		return nil, err
+func (w withNotification) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact, labellers []deploy.Labeller) error {
+	if err := w.Deployer.Deploy(ctx, out, builds, labellers); err != nil {
+		return err
 	}
 
 	fmt.Fprint(out, terminalBell)
 
-	return res, nil
+	return nil
 }

pkg/skaffold/runner/runner.go

@@ -52,6 +52,7 @@ type SkaffoldRunner struct {
 	watch.Watcher
 
 	opts        *config.SkaffoldOptions
+	labellers   []deploy.Labeller
 	builds      []build.Artifact
 	hasDeployed bool
 	imageList   *kubernetes.ImageList
@@ -90,7 +91,8 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (*
 		return nil, errors.Wrap(err, "parsing deploy config")
 	}
 
-	deployer = deploy.WithLabels(deployer, opts, builder, deployer, tagger)
+	labellers := []deploy.Labeller{opts, builder, deployer, tagger}
+
 	builder, tester, deployer = WithTimings(builder, tester, deployer)
 	if opts.Notification {
 		deployer = WithNotification(deployer)
@@ -109,6 +111,7 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (*
 		Syncer:    &kubectl.Syncer{},
 		Watcher:   watch.NewWatcher(trigger),
 		opts:      opts,
+		labellers: labellers,
 		imageList: kubernetes.NewImageList(),
 	}, nil
 }
@@ -220,7 +223,7 @@ func (r *SkaffoldRunner) buildTestDeploy(ctx context.Context, out io.Writer, art
 	// Make sure all artifacts are redeployed. Not only those that were just built.
 	r.builds = mergeWithPreviousBuilds(bRes, r.builds)
 
-	if _, err := r.Deploy(ctx, out, r.builds); err != nil {
+	if err := r.Deploy(ctx, out, r.builds); err != nil {
 		return errors.Wrap(err, "deploy failed")
 	}
 
@@ -260,10 +263,10 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 }
 
 // Deploy deploys the given artifacts
-func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) ([]deploy.Artifact, error) {
-	dRes, err := r.Deployer.Deploy(ctx, out, artifacts)
+func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) error {
+	err := r.Deployer.Deploy(ctx, out, artifacts, r.labellers)
 	r.hasDeployed = true
-	return dRes, err
+	return err
 }
 
 // TailLogs prints the logs for deployed artifacts.

pkg/skaffold/runner/runner_test.go

@@ -115,17 +115,17 @@ func (t *TestBench) Test(ctx context.Context, out io.Writer, artifacts []build.A
 	return nil
 }
 
-func (t *TestBench) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) ([]deploy.Artifact, error) {
+func (t *TestBench) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact, labellers []deploy.Labeller) error {
 	if len(t.deployErrors) > 0 {
 		err := t.deployErrors[0]
 		t.deployErrors = t.deployErrors[1:]
 		if err != nil {
-			return nil, err
+			return err
 		}
 	}
 
 	t.currentActions.Deployed = tags(artifacts)
-	return nil, nil
+	return nil
 }
 
 func (t *TestBench) Actions() []Actions {

pkg/skaffold/runner/timings.go

@@ -78,17 +78,16 @@ func (w withTimings) Test(ctx context.Context, out io.Writer, builds []build.Art
 	return nil
 }
 
-func (w withTimings) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]deploy.Artifact, error) {
+func (w withTimings) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact, labellers []deploy.Labeller) error {
 	start := time.Now()
 	color.Default.Fprintln(out, "Starting deploy...")
 
-	dRes, err := w.Deployer.Deploy(ctx, out, builds)
-	if err != nil {
-		return nil, err
+	if err := w.Deployer.Deploy(ctx, out, builds, labellers); err != nil {
+		return err
 	}
 
 	color.Default.Fprintln(out, "Deploy complete in", time.Since(start))
-	return dRes, nil
+	return nil
 }
 
 func (w withTimings) Cleanup(ctx context.Context, out io.Writer) error {