Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embed labelling into Deployers #1463

Merged
merged 1 commit into from
Jan 17, 2019
Merged

Embed labelling into Deployers #1463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/skaffold/deploy/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"io"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"

"k8s.io/apimachinery/pkg/runtime"
)

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of passing the labellers as a parameter to the Deploy() call, could we just embed the labellers themselves into the deployer object when we instantiate the deployer in the runner? imo the simpler the method signature the better, especially for something that might be exposed externally (for plugins) in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to go that way first but I felt like labelling is really part of the Deployer's contract so should be visible here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm ok I see your reasoning. sgtm


// Dependencies returns a list of files that the deployer depends on.
// In dev mode, a redeploy will be triggered
Expand Down
16 changes: 11 additions & 5 deletions pkg/skaffold/deploy/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/deploy/helm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
Expand Down
20 changes: 12 additions & 8 deletions pkg/skaffold/deploy/kubectl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
14 changes: 7 additions & 7 deletions pkg/skaffold/deploy/kubectl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
Expand Down Expand Up @@ -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)
}
16 changes: 10 additions & 6 deletions pkg/skaffold/deploy/kustomize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
27 changes: 0 additions & 27 deletions pkg/skaffold/deploy/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/runner/dev.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,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
}
Expand Down
9 changes: 4 additions & 5 deletions pkg/skaffold/runner/notification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
13 changes: 8 additions & 5 deletions pkg/skaffold/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type SkaffoldRunner struct {
watch.Watcher

opts *config.SkaffoldOptions
labellers []deploy.Labeller
builds []build.Artifact
hasDeployed bool
imageList *kubernetes.ImageList
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions pkg/skaffold/runner/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 4 additions & 5 deletions pkg/skaffold/runner/timings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down