From 47dcc9ae8a1325cfc7e65555aae036c662a974c3 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 16 Aug 2018 10:44:06 +0200 Subject: [PATCH 1/4] Make kustomize deployer more similar to kubectl deployer This way, it should be easier to remove duplication Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl.go | 21 +++++----- pkg/skaffold/deploy/kustomize.go | 66 ++++++++++++++++---------------- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index 69313162fc2..7280dc209e1 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -93,9 +93,8 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu return nil, nil } - err = k.kubectl.Run(updated.reader(), out, "apply", k.Flags.Apply, "-f", "-") - if err != nil { - return nil, errors.Wrap(err, "deploying manifests") + if err := k.kubectl.Run(manifests.reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { + return nil, errors.Wrap(err, "kubectl apply") } return parseManifestsForDeploys(updated) @@ -109,7 +108,7 @@ func (k *KubectlDeployer) Cleanup(ctx context.Context, out io.Writer) error { } if err := k.kubectl.Run(manifests.reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { - return errors.Wrap(err, "deleting manifests") + return errors.Wrap(err, "kubectl delete") } return nil @@ -155,18 +154,15 @@ func (k *KubectlDeployer) readManifests() (manifestList, error) { if err != nil { return nil, errors.Wrap(err, "expanding user manifest list") } - var manifests manifestList + var manifests manifestList for _, manifest := range files { buf, err := ioutil.ReadFile(manifest) if err != nil { return nil, errors.Wrap(err, "reading manifest") } - parts := bytes.Split(buf, []byte("\n---")) - for _, part := range parts { - manifests = append(manifests, part) - } + manifests.append(buf) } for _, m := range k.RemoteManifests { @@ -222,6 +218,13 @@ func (l *manifestList) Empty() bool { return len(*l) == 0 } +func (l *manifestList) append(buf []byte) { + parts := bytes.Split(buf, []byte("\n---")) + for _, part := range parts { + *l = append(*l, part) + } +} + func (l *manifestList) diff(manifests manifestList) manifestList { if l == nil { return manifests diff --git a/pkg/skaffold/deploy/kustomize.go b/pkg/skaffold/deploy/kustomize.go index 4de2d42c654..713a70a041d 100644 --- a/pkg/skaffold/deploy/kustomize.go +++ b/pkg/skaffold/deploy/kustomize.go @@ -17,10 +17,8 @@ limitations under the License. package deploy import ( - "bytes" "context" "io" - "io/ioutil" "os/exec" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" @@ -29,12 +27,14 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) type KustomizeDeployer struct { *v1alpha2.KustomizeDeploy - kubectl kubectl.CLI + kubectl kubectl.CLI + previousDeployment manifestList } func NewKustomizeDeployer(cfg *v1alpha2.KustomizeDeploy, kubeContext string, namespace string) *KustomizeDeployer { @@ -55,47 +55,46 @@ func (k *KustomizeDeployer) Labels() map[string]string { } func (k *KustomizeDeployer) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]Artifact, error) { - manifests, err := buildManifests(k.KustomizePath) + manifests, err := k.readManifests() if err != nil { - return nil, errors.Wrap(err, "kustomize") + return nil, errors.Wrap(err, "reading manifests") } - manifestList, err := newManifestList(manifests) - if err != nil { - return nil, errors.Wrap(err, "getting manifest list") + + if manifests.Empty() { + return nil, nil } - manifestList, err = manifestList.replaceImages(builds) + + manifests, err = manifests.replaceImages(builds) if err != nil { - return nil, errors.Wrap(err, "replacing images") + return nil, errors.Wrap(err, "replacing images in manifests") } - if err := k.kubectl.Run(manifestList.reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { - return nil, errors.Wrap(err, "running kubectl") - } - return parseManifestsForDeploys(manifestList) -} -func newManifestList(r io.Reader) (manifestList, error) { - var manifests manifestList - buf, err := ioutil.ReadAll(r) - if err != nil { - return nil, errors.Wrap(err, "reading manifests") + // Only redeploy modified or new manifests + // TODO(dgageot): should we delete a manifest that was deployed and is not anymore? + updated := k.previousDeployment.diff(manifests) + logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new") + k.previousDeployment = manifests + if len(updated) == 0 { + return nil, nil } - parts := bytes.Split(buf, []byte("\n---")) - for _, part := range parts { - manifests = append(manifests, part) + if err := k.kubectl.Run(manifests.reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { + return nil, errors.Wrap(err, "kubectl apply") } - return manifests, nil + return parseManifestsForDeploys(updated) } func (k *KustomizeDeployer) Cleanup(ctx context.Context, out io.Writer) error { - manifests, err := buildManifests(k.KustomizePath) + manifests, err := k.readManifests() if err != nil { - return errors.Wrap(err, "kustomize") + return errors.Wrap(err, "reading manifests") } - if err := k.kubectl.Run(manifests, out, "delete", k.Flags.Delete, "-f", "-"); err != nil { + + if err := k.kubectl.Run(manifests.reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { return errors.Wrap(err, "kubectl delete") } + return nil } @@ -104,11 +103,14 @@ func (k *KustomizeDeployer) Dependencies() ([]string, error) { return []string{k.KustomizePath}, nil } -func buildManifests(kustomization string) (io.Reader, error) { - cmd := exec.Command("kustomize", "build", kustomization) - out, err := util.DefaultExecCommand.RunCmdOut(cmd) +func (k *KustomizeDeployer) readManifests() (manifestList, error) { + cmd := exec.Command("kustomize", "build", k.KustomizePath) + out, err := util.RunCmdOut(cmd) if err != nil { - return nil, errors.Wrap(err, "running kustomize build") + return nil, errors.Wrap(err, "kustomize build") } - return bytes.NewReader(out), nil + + var manifests manifestList + manifests.append(out) + return manifests, nil } From 736eacb309ba83e3c8e7fe8dd01ee19c2cfea3be Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 16 Aug 2018 11:11:18 +0200 Subject: [PATCH 2/4] Extract ManifestList Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl.go | 156 ++--------------- pkg/skaffold/deploy/kubectl/manifests.go | 162 ++++++++++++++++++ pkg/skaffold/deploy/kubectl/manifests_test.go | 125 ++++++++++++++ pkg/skaffold/deploy/{ => kubectl}/warnings.go | 2 +- pkg/skaffold/deploy/kubectl_test.go | 100 ----------- pkg/skaffold/deploy/kustomize.go | 18 +- 6 files changed, 308 insertions(+), 255 deletions(-) create mode 100644 pkg/skaffold/deploy/kubectl/manifests.go create mode 100644 pkg/skaffold/deploy/kubectl/manifests_test.go rename pkg/skaffold/deploy/{ => kubectl}/warnings.go (98%) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index 7280dc209e1..fa737a5b678 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -27,24 +27,19 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/kubectl" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/pkg/errors" "github.com/sirupsen/logrus" - "gopkg.in/yaml.v2" ) -// for testing -var warner Warner = &logrusWarner{} - // KubectlDeployer deploys workflows using kubectl CLI. type KubectlDeployer struct { *v1alpha2.KubectlDeploy - kubectl kubectl.CLI workingDir string - previousDeployment manifestList + kubectl kubectl.CLI + previousDeployment kubectl.ManifestList } // NewKubectlDeployer returns a new KubectlDeployer for a DeployConfig filled @@ -75,25 +70,25 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu return nil, errors.Wrap(err, "reading manifests") } - if manifests.Empty() { + if len(manifests) == 0 { return nil, nil } - manifests, err = manifests.replaceImages(builds) + manifests, err = manifests.ReplaceImages(builds) if err != nil { return nil, errors.Wrap(err, "replacing images in manifests") } // Only redeploy modified or new manifests // TODO(dgageot): should we delete a manifest that was deployed and is not anymore? - updated := k.previousDeployment.diff(manifests) + updated := k.previousDeployment.Diff(manifests) logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new") k.previousDeployment = manifests if len(updated) == 0 { return nil, nil } - if err := k.kubectl.Run(manifests.reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { + if err := k.kubectl.Run(manifests.Reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { return nil, errors.Wrap(err, "kubectl apply") } @@ -107,7 +102,7 @@ func (k *KubectlDeployer) Cleanup(ctx context.Context, out io.Writer) error { return errors.Wrap(err, "reading manifests") } - if err := k.kubectl.Run(manifests.reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { + if err := k.kubectl.Run(manifests.Reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { return errors.Wrap(err, "kubectl delete") } @@ -139,7 +134,7 @@ func (k *KubectlDeployer) manifestFiles(manifests []string) ([]string, error) { return filteredManifests, nil } -func parseManifestsForDeploys(manifests manifestList) ([]Artifact, error) { +func parseManifestsForDeploys(manifests kubectl.ManifestList) ([]Artifact, error) { results := []Artifact{} for _, manifest := range manifests { b := bufio.NewReader(bytes.NewReader(manifest)) @@ -149,20 +144,20 @@ func parseManifestsForDeploys(manifests manifestList) ([]Artifact, error) { } // readManifests reads the manifests to deploy/delete. -func (k *KubectlDeployer) readManifests() (manifestList, error) { +func (k *KubectlDeployer) readManifests() (kubectl.ManifestList, error) { files, err := k.manifestFiles(k.Manifests) if err != nil { return nil, errors.Wrap(err, "expanding user manifest list") } - var manifests manifestList + var manifests kubectl.ManifestList for _, manifest := range files { buf, err := ioutil.ReadFile(manifest) if err != nil { return nil, errors.Wrap(err, "reading manifest") } - manifests.append(buf) + manifests.Append(buf) } for _, m := range k.RemoteManifests { @@ -195,132 +190,3 @@ func (k *KubectlDeployer) readRemoteManifest(name string) ([]byte, error) { return manifest.Bytes(), nil } - -type replacement struct { - tag string - found bool -} - -type manifestList [][]byte - -func (l *manifestList) String() string { - var str string - for i, manifest := range *l { - if i != 0 { - str += "\n---\n" - } - str += string(bytes.TrimSpace(manifest)) - } - return str -} - -func (l *manifestList) Empty() bool { - return len(*l) == 0 -} - -func (l *manifestList) append(buf []byte) { - parts := bytes.Split(buf, []byte("\n---")) - for _, part := range parts { - *l = append(*l, part) - } -} - -func (l *manifestList) diff(manifests manifestList) manifestList { - if l == nil { - return manifests - } - - oldManifests := map[string]bool{} - for _, oldManifest := range *l { - oldManifests[string(oldManifest)] = true - } - - var updated manifestList - - for _, manifest := range manifests { - if !oldManifests[string(manifest)] { - updated = append(updated, manifest) - } - } - - return updated -} - -func (l *manifestList) reader() io.Reader { - return strings.NewReader(l.String()) -} - -func (l *manifestList) replaceImages(builds []build.Artifact) (manifestList, error) { - replacements := map[string]*replacement{} - for _, build := range builds { - replacements[build.ImageName] = &replacement{ - tag: build.Tag, - } - } - - var updatedManifests manifestList - - for _, manifest := range *l { - m := make(map[interface{}]interface{}) - if err := yaml.Unmarshal(manifest, &m); err != nil { - return nil, errors.Wrap(err, "reading kubernetes YAML") - } - - if len(m) == 0 { - continue - } - - recursiveReplaceImage(m, replacements) - - updatedManifest, err := yaml.Marshal(m) - if err != nil { - return nil, errors.Wrap(err, "marshalling yaml") - } - - updatedManifests = append(updatedManifests, updatedManifest) - } - - for name, replacement := range replacements { - if !replacement.found { - warner.Warnf("image [%s] is not used by the deployment", name) - } - } - - logrus.Debugln("manifests with tagged images", updatedManifests.String()) - - return updatedManifests, nil -} - -func recursiveReplaceImage(i interface{}, replacements map[string]*replacement) { - switch t := i.(type) { - case []interface{}: - for _, v := range t { - recursiveReplaceImage(v, replacements) - } - case map[interface{}]interface{}: - for k, v := range t { - if k.(string) != "image" { - recursiveReplaceImage(v, replacements) - continue - } - - image := v.(string) - parsed, err := docker.ParseReference(image) - if err != nil { - warner.Warnf("Couldn't parse image: %s", v) - continue - } - - if img, present := replacements[parsed.BaseName]; present { - if parsed.FullyQualified { - if img.tag == image { - img.found = true - } - } else { - t[k] = img.tag - img.found = true - } - } - } - } -} diff --git a/pkg/skaffold/deploy/kubectl/manifests.go b/pkg/skaffold/deploy/kubectl/manifests.go new file mode 100644 index 00000000000..7e4c93ebf93 --- /dev/null +++ b/pkg/skaffold/deploy/kubectl/manifests.go @@ -0,0 +1,162 @@ +/* +Copyright 2018 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package kubectl + +import ( + "bytes" + "io" + "strings" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + "gopkg.in/yaml.v2" +) + +// for testing +var warner Warner = &logrusWarner{} + +// ManifestList is a list of yaml manifests. +type ManifestList [][]byte + +func (l *ManifestList) String() string { + var str string + for i, manifest := range *l { + if i != 0 { + str += "\n---\n" + } + str += string(bytes.TrimSpace(manifest)) + } + return str +} + +// Append appends the yaml manifests defined in the given buffer. +func (l *ManifestList) Append(buf []byte) { + parts := bytes.Split(buf, []byte("\n---")) + for _, part := range parts { + *l = append(*l, part) + } +} + +// Diff computes the list of manifests that have changed. +func (l *ManifestList) Diff(latest ManifestList) ManifestList { + if l == nil { + return latest + } + + oldManifests := map[string]bool{} + for _, oldManifest := range *l { + oldManifests[string(oldManifest)] = true + } + + var updated ManifestList + + for _, manifest := range latest { + if !oldManifests[string(manifest)] { + updated = append(updated, manifest) + } + } + + return updated +} + +// Reader returns a reader on the raw yaml descriptors. +func (l *ManifestList) Reader() io.Reader { + return strings.NewReader(l.String()) +} + +type replacement struct { + tag string + found bool +} + +// ReplaceImages replaces the image names in the manifests. +func (l *ManifestList) ReplaceImages(builds []build.Artifact) (ManifestList, error) { + replacements := map[string]*replacement{} + for _, build := range builds { + replacements[build.ImageName] = &replacement{ + tag: build.Tag, + } + } + + var updatedManifests ManifestList + + for _, manifest := range *l { + m := make(map[interface{}]interface{}) + if err := yaml.Unmarshal(manifest, &m); err != nil { + return nil, errors.Wrap(err, "reading kubernetes YAML") + } + + if len(m) == 0 { + continue + } + + recursiveReplaceImage(m, replacements) + + updatedManifest, err := yaml.Marshal(m) + if err != nil { + return nil, errors.Wrap(err, "marshalling yaml") + } + + updatedManifests = append(updatedManifests, updatedManifest) + } + + for name, replacement := range replacements { + if !replacement.found { + warner.Warnf("image [%s] is not used by the deployment", name) + } + } + + logrus.Debugln("manifests with tagged images", updatedManifests.String()) + + return updatedManifests, nil +} + +func recursiveReplaceImage(i interface{}, replacements map[string]*replacement) { + switch t := i.(type) { + case []interface{}: + for _, v := range t { + recursiveReplaceImage(v, replacements) + } + case map[interface{}]interface{}: + for k, v := range t { + if k.(string) != "image" { + recursiveReplaceImage(v, replacements) + continue + } + + image := v.(string) + parsed, err := docker.ParseReference(image) + if err != nil { + warner.Warnf("Couldn't parse image: %s", v) + continue + } + + if img, present := replacements[parsed.BaseName]; present { + if parsed.FullyQualified { + if img.tag == image { + img.found = true + } + } else { + t[k] = img.tag + img.found = true + } + } + } + } +} diff --git a/pkg/skaffold/deploy/kubectl/manifests_test.go b/pkg/skaffold/deploy/kubectl/manifests_test.go new file mode 100644 index 00000000000..6bc9f26419b --- /dev/null +++ b/pkg/skaffold/deploy/kubectl/manifests_test.go @@ -0,0 +1,125 @@ +/* +Copyright 2018 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package kubectl + +import ( + "fmt" + "sort" + "testing" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" + "github.com/GoogleContainerTools/skaffold/testutil" +) + +type fakeWarner struct { + warnings []string +} + +func (l *fakeWarner) Warnf(format string, args ...interface{}) { + l.warnings = append(l.warnings, fmt.Sprintf(format, args...)) + sort.Strings(l.warnings) +} + +func TestReplaceImages(t *testing.T) { + manifests := ManifestList{[]byte(` +apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: gcr.io/k8s-skaffold/example + name: not-tagged + - image: gcr.io/k8s-skaffold/example:latest + name: latest + - image: gcr.io/k8s-skaffold/example:v1 + name: fully-qualified + - image: skaffold/other + name: other + - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 + name: digest + - image: skaffold/usedbyfqn:TAG + - image: skaffold/usedwrongfqn:OTHER +`)} + + builds := []build.Artifact{{ + ImageName: "gcr.io/k8s-skaffold/example", + Tag: "gcr.io/k8s-skaffold/example:TAG", + }, { + ImageName: "skaffold/other", + Tag: "skaffold/other:OTHER_TAG", + }, { + ImageName: "skaffold/unused", + Tag: "skaffold/unused:TAG", + }, { + ImageName: "skaffold/usedbyfqn", + Tag: "skaffold/usedbyfqn:TAG", + }, { + ImageName: "skaffold/usedwrongfqn", + Tag: "skaffold/usedwrongfqn:TAG", + }} + + expected := ManifestList{[]byte(` +apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: gcr.io/k8s-skaffold/example:TAG + name: not-tagged + - image: gcr.io/k8s-skaffold/example:TAG + name: latest + - image: gcr.io/k8s-skaffold/example:v1 + name: fully-qualified + - image: skaffold/other:OTHER_TAG + name: other + - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 + name: digest + - image: skaffold/usedbyfqn:TAG + - image: skaffold/usedwrongfqn:OTHER +`)} + + defer func(w Warner) { warner = w }(warner) + fakeWarner := &fakeWarner{} + warner = fakeWarner + + resultManifest, err := manifests.ReplaceImages(builds) + + testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String()) + testutil.CheckErrorAndDeepEqual(t, false, err, []string{ + "image [skaffold/unused] is not used by the deployment", + "image [skaffold/usedwrongfqn] is not used by the deployment", + }, fakeWarner.warnings) +} + +func TestReplaceEmptyManifest(t *testing.T) { + manifests := ManifestList{[]byte(""), []byte(" ")} + expected := ManifestList{} + + resultManifest, err := manifests.ReplaceImages(nil) + + testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String()) +} + +func TestReplaceInvalidManifest(t *testing.T) { + manifests := ManifestList{[]byte("INVALID")} + + _, err := manifests.ReplaceImages(nil) + + testutil.CheckError(t, true, err) +} diff --git a/pkg/skaffold/deploy/warnings.go b/pkg/skaffold/deploy/kubectl/warnings.go similarity index 98% rename from pkg/skaffold/deploy/warnings.go rename to pkg/skaffold/deploy/kubectl/warnings.go index 0d8c3f8796a..40738379ec3 100644 --- a/pkg/skaffold/deploy/warnings.go +++ b/pkg/skaffold/deploy/kubectl/warnings.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package deploy +package kubectl import "github.com/sirupsen/logrus" diff --git a/pkg/skaffold/deploy/kubectl_test.go b/pkg/skaffold/deploy/kubectl_test.go index 232a05c4cc0..8b54f9ac207 100644 --- a/pkg/skaffold/deploy/kubectl_test.go +++ b/pkg/skaffold/deploy/kubectl_test.go @@ -20,7 +20,6 @@ import ( "bytes" "context" "fmt" - "sort" "testing" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" @@ -54,15 +53,6 @@ spec: ports: - containerPort: 8080` -type fakeWarner struct { - warnings []string -} - -func (l *fakeWarner) Warnf(format string, args ...interface{}) { - l.warnings = append(l.warnings, fmt.Sprintf(format, args...)) - sort.Strings(l.warnings) -} - func TestKubectlDeploy(t *testing.T) { var tests = []struct { description string @@ -220,93 +210,3 @@ func TestKubectlCleanup(t *testing.T) { }) } } - -func TestReplaceImages(t *testing.T) { - manifests := manifestList{[]byte(` -apiVersion: v1 -kind: Pod -metadata: - name: getting-started -spec: - containers: - - image: gcr.io/k8s-skaffold/example - name: not-tagged - - image: gcr.io/k8s-skaffold/example:latest - name: latest - - image: gcr.io/k8s-skaffold/example:v1 - name: fully-qualified - - image: skaffold/other - name: other - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: digest - - image: skaffold/usedbyfqn:TAG - - image: skaffold/usedwrongfqn:OTHER -`)} - - builds := []build.Artifact{{ - ImageName: "gcr.io/k8s-skaffold/example", - Tag: "gcr.io/k8s-skaffold/example:TAG", - }, { - ImageName: "skaffold/other", - Tag: "skaffold/other:OTHER_TAG", - }, { - ImageName: "skaffold/unused", - Tag: "skaffold/unused:TAG", - }, { - ImageName: "skaffold/usedbyfqn", - Tag: "skaffold/usedbyfqn:TAG", - }, { - ImageName: "skaffold/usedwrongfqn", - Tag: "skaffold/usedwrongfqn:TAG", - }} - - expected := manifestList{[]byte(` -apiVersion: v1 -kind: Pod -metadata: - name: getting-started -spec: - containers: - - image: gcr.io/k8s-skaffold/example:TAG - name: not-tagged - - image: gcr.io/k8s-skaffold/example:TAG - name: latest - - image: gcr.io/k8s-skaffold/example:v1 - name: fully-qualified - - image: skaffold/other:OTHER_TAG - name: other - - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 - name: digest - - image: skaffold/usedbyfqn:TAG - - image: skaffold/usedwrongfqn:OTHER -`)} - - defer func(w Warner) { warner = w }(warner) - fakeWarner := &fakeWarner{} - warner = fakeWarner - - resultManifest, err := manifests.replaceImages(builds) - - testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String()) - testutil.CheckErrorAndDeepEqual(t, false, err, []string{ - "image [skaffold/unused] is not used by the deployment", - "image [skaffold/usedwrongfqn] is not used by the deployment", - }, fakeWarner.warnings) -} - -func TestReplaceEmptyManifest(t *testing.T) { - manifests := manifestList{[]byte(""), []byte(" ")} - expected := manifestList{} - - resultManifest, err := manifests.replaceImages(nil) - - testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String()) -} - -func TestReplaceInvalidManifest(t *testing.T) { - manifests := manifestList{[]byte("INVALID")} - - _, err := manifests.replaceImages(nil) - - testutil.CheckError(t, true, err) -} diff --git a/pkg/skaffold/deploy/kustomize.go b/pkg/skaffold/deploy/kustomize.go index 713a70a041d..b119f970ad3 100644 --- a/pkg/skaffold/deploy/kustomize.go +++ b/pkg/skaffold/deploy/kustomize.go @@ -34,7 +34,7 @@ type KustomizeDeployer struct { *v1alpha2.KustomizeDeploy kubectl kubectl.CLI - previousDeployment manifestList + previousDeployment kubectl.ManifestList } func NewKustomizeDeployer(cfg *v1alpha2.KustomizeDeploy, kubeContext string, namespace string) *KustomizeDeployer { @@ -60,25 +60,25 @@ func (k *KustomizeDeployer) Deploy(ctx context.Context, out io.Writer, builds [] return nil, errors.Wrap(err, "reading manifests") } - if manifests.Empty() { + if len(manifests) == 0 { return nil, nil } - manifests, err = manifests.replaceImages(builds) + manifests, err = manifests.ReplaceImages(builds) if err != nil { return nil, errors.Wrap(err, "replacing images in manifests") } // Only redeploy modified or new manifests // TODO(dgageot): should we delete a manifest that was deployed and is not anymore? - updated := k.previousDeployment.diff(manifests) + updated := k.previousDeployment.Diff(manifests) logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new") k.previousDeployment = manifests if len(updated) == 0 { return nil, nil } - if err := k.kubectl.Run(manifests.reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { + if err := k.kubectl.Run(manifests.Reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { return nil, errors.Wrap(err, "kubectl apply") } @@ -91,7 +91,7 @@ func (k *KustomizeDeployer) Cleanup(ctx context.Context, out io.Writer) error { return errors.Wrap(err, "reading manifests") } - if err := k.kubectl.Run(manifests.reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { + if err := k.kubectl.Run(manifests.Reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { return errors.Wrap(err, "kubectl delete") } @@ -103,14 +103,14 @@ func (k *KustomizeDeployer) Dependencies() ([]string, error) { return []string{k.KustomizePath}, nil } -func (k *KustomizeDeployer) readManifests() (manifestList, error) { +func (k *KustomizeDeployer) readManifests() (kubectl.ManifestList, error) { cmd := exec.Command("kustomize", "build", k.KustomizePath) out, err := util.RunCmdOut(cmd) if err != nil { return nil, errors.Wrap(err, "kustomize build") } - var manifests manifestList - manifests.append(out) + var manifests kubectl.ManifestList + manifests.Append(out) return manifests, nil } From ca7eee2e6d389a3e22cff7450e077c447a35d6fd Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 16 Aug 2018 11:20:37 +0200 Subject: [PATCH 3/4] Remove duplication of `kubectl delete` Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl.go | 6 +----- pkg/skaffold/deploy/kubectl/cli.go | 10 ++++++++++ pkg/skaffold/deploy/kustomize.go | 6 +----- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index fa737a5b678..d6495544a8b 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -102,11 +102,7 @@ func (k *KubectlDeployer) Cleanup(ctx context.Context, out io.Writer) error { return errors.Wrap(err, "reading manifests") } - if err := k.kubectl.Run(manifests.Reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { - return errors.Wrap(err, "kubectl delete") - } - - return nil + return k.kubectl.Detete(out, manifests) } func (k *KubectlDeployer) Dependencies() ([]string, error) { diff --git a/pkg/skaffold/deploy/kubectl/cli.go b/pkg/skaffold/deploy/kubectl/cli.go index 169cad1e628..42dd1943f42 100644 --- a/pkg/skaffold/deploy/kubectl/cli.go +++ b/pkg/skaffold/deploy/kubectl/cli.go @@ -22,6 +22,7 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" + "github.com/pkg/errors" ) // CLI holds parameters to run kubectl. @@ -31,6 +32,15 @@ type CLI struct { Flags v1alpha2.KubectlFlags } +// Detete runs `kubectl delete` on a list of manifests. +func (c *CLI) Detete(out io.Writer, manifests ManifestList) error { + if err := c.Run(manifests.Reader(), out, "delete", c.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { + return errors.Wrap(err, "kubectl delete") + } + + return nil +} + // Run shells out kubectl CLI. func (c *CLI) Run(in io.Reader, out io.Writer, command string, commandFlags []string, arg ...string) error { args := []string{"--context", c.KubeContext} diff --git a/pkg/skaffold/deploy/kustomize.go b/pkg/skaffold/deploy/kustomize.go index b119f970ad3..4cc85d19111 100644 --- a/pkg/skaffold/deploy/kustomize.go +++ b/pkg/skaffold/deploy/kustomize.go @@ -91,11 +91,7 @@ func (k *KustomizeDeployer) Cleanup(ctx context.Context, out io.Writer) error { return errors.Wrap(err, "reading manifests") } - if err := k.kubectl.Run(manifests.Reader(), out, "delete", k.Flags.Delete, "--ignore-not-found=true", "-f", "-"); err != nil { - return errors.Wrap(err, "kubectl delete") - } - - return nil + return k.kubectl.Detete(out, manifests) } func (k *KustomizeDeployer) Dependencies() ([]string, error) { From 8a12a3af6a9fc2db1c67c36490aab8f9f0e3e45b Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 16 Aug 2018 11:28:26 +0200 Subject: [PATCH 4/4] Remove duplication of `kubectl apply` Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl.go | 25 ++++++++++--------------- pkg/skaffold/deploy/kubectl/cli.go | 21 +++++++++++++++++++++ pkg/skaffold/deploy/kustomize.go | 24 +++++++++--------------- 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index d6495544a8b..3479af07972 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -37,9 +37,8 @@ import ( type KubectlDeployer struct { *v1alpha2.KubectlDeploy - workingDir string - kubectl kubectl.CLI - previousDeployment kubectl.ManifestList + workingDir string + kubectl kubectl.CLI } // NewKubectlDeployer returns a new KubectlDeployer for a DeployConfig filled @@ -79,17 +78,9 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu return nil, errors.Wrap(err, "replacing images in manifests") } - // Only redeploy modified or new manifests - // TODO(dgageot): should we delete a manifest that was deployed and is not anymore? - updated := k.previousDeployment.Diff(manifests) - logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new") - k.previousDeployment = manifests - if len(updated) == 0 { - return nil, nil - } - - if err := k.kubectl.Run(manifests.Reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { - return nil, errors.Wrap(err, "kubectl apply") + updated, err := k.kubectl.Apply(out, manifests) + if err != nil { + return nil, errors.Wrap(err, "apply") } return parseManifestsForDeploys(updated) @@ -102,7 +93,11 @@ func (k *KubectlDeployer) Cleanup(ctx context.Context, out io.Writer) error { return errors.Wrap(err, "reading manifests") } - return k.kubectl.Detete(out, manifests) + if err := k.kubectl.Detete(out, manifests); err != nil { + return errors.Wrap(err, "delete") + } + + return nil } func (k *KubectlDeployer) Dependencies() ([]string, error) { diff --git a/pkg/skaffold/deploy/kubectl/cli.go b/pkg/skaffold/deploy/kubectl/cli.go index 42dd1943f42..e534f7efb77 100644 --- a/pkg/skaffold/deploy/kubectl/cli.go +++ b/pkg/skaffold/deploy/kubectl/cli.go @@ -23,6 +23,7 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) // CLI holds parameters to run kubectl. @@ -30,6 +31,8 @@ type CLI struct { Namespace string KubeContext string Flags v1alpha2.KubectlFlags + + previousApply ManifestList } // Detete runs `kubectl delete` on a list of manifests. @@ -41,6 +44,24 @@ func (c *CLI) Detete(out io.Writer, manifests ManifestList) error { return nil } +// Apply runs `kubectl apply` on a list of manifests. +func (c *CLI) Apply(out io.Writer, manifests ManifestList) (ManifestList, error) { + // Only redeploy modified or new manifests + // TODO(dgageot): should we delete a manifest that was deployed and is not anymore? + updated := c.previousApply.Diff(manifests) + logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new") + c.previousApply = manifests + if len(updated) == 0 { + return nil, nil + } + + if err := c.Run(manifests.Reader(), out, "apply", c.Flags.Apply, "-f", "-"); err != nil { + return nil, errors.Wrap(err, "kubectl apply") + } + + return updated, nil +} + // Run shells out kubectl CLI. func (c *CLI) Run(in io.Reader, out io.Writer, command string, commandFlags []string, arg ...string) error { args := []string{"--context", c.KubeContext} diff --git a/pkg/skaffold/deploy/kustomize.go b/pkg/skaffold/deploy/kustomize.go index 4cc85d19111..24277f72670 100644 --- a/pkg/skaffold/deploy/kustomize.go +++ b/pkg/skaffold/deploy/kustomize.go @@ -27,14 +27,12 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/pkg/errors" - "github.com/sirupsen/logrus" ) type KustomizeDeployer struct { *v1alpha2.KustomizeDeploy - kubectl kubectl.CLI - previousDeployment kubectl.ManifestList + kubectl kubectl.CLI } func NewKustomizeDeployer(cfg *v1alpha2.KustomizeDeploy, kubeContext string, namespace string) *KustomizeDeployer { @@ -69,17 +67,9 @@ func (k *KustomizeDeployer) Deploy(ctx context.Context, out io.Writer, builds [] return nil, errors.Wrap(err, "replacing images in manifests") } - // Only redeploy modified or new manifests - // TODO(dgageot): should we delete a manifest that was deployed and is not anymore? - updated := k.previousDeployment.Diff(manifests) - logrus.Debugln(len(manifests), "manifests to deploy.", len(manifests), "are updated or new") - k.previousDeployment = manifests - if len(updated) == 0 { - return nil, nil - } - - if err := k.kubectl.Run(manifests.Reader(), out, "apply", k.Flags.Apply, "-f", "-"); err != nil { - return nil, errors.Wrap(err, "kubectl apply") + updated, err := k.kubectl.Apply(out, manifests) + if err != nil { + return nil, errors.Wrap(err, "apply") } return parseManifestsForDeploys(updated) @@ -91,7 +81,11 @@ func (k *KustomizeDeployer) Cleanup(ctx context.Context, out io.Writer) error { return errors.Wrap(err, "reading manifests") } - return k.kubectl.Detete(out, manifests) + if err := k.kubectl.Detete(out, manifests); err != nil { + return errors.Wrap(err, "delete") + } + + return nil } func (k *KustomizeDeployer) Dependencies() ([]string, error) {