Decouple visiting manifests and replacing images

pkg/skaffold/deploy/kubectl/images.go

@@ -0,0 +1,95 @@
+/*
+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 (
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+)
+
+// for testing
+var warner Warner = &logrusWarner{}
+
+// ReplaceImages replaces image names in a list of manifests.
+func (l *ManifestList) ReplaceImages(builds []build.Artifact) (ManifestList, error) {
+	replacer := newImageReplacer(builds)
+
+	updated, err := l.Visit(replacer)
+	if err != nil {
+		return nil, errors.Wrap(err, "replacing images")
+	}
+
+	replacer.Check()
+	logrus.Debugln("manifests with tagged images", updated.String())
+
+	return updated, nil
+}
+
+type imageReplacer struct {
+	tagsByImageName map[string]string
+	found           map[string]bool
+}
+
+func newImageReplacer(builds []build.Artifact) *imageReplacer {
+	tagsByImageName := make(map[string]string)
+	for _, build := range builds {
+		tagsByImageName[build.ImageName] = build.Tag
+	}
+
+	return &imageReplacer{
+		tagsByImageName: tagsByImageName,
+		found:           make(map[string]bool),
+	}
+}
+
+func (r *imageReplacer) Matches(key string) bool {
+	return key == "image"
+}
+
+func (r *imageReplacer) NewValue(key string, old interface{}) (bool, interface{}) {
+	image := old.(string)
+
+	parsed, err := docker.ParseReference(image)
+	if err != nil {
+		warner.Warnf("Couldn't parse image: %s", image)
+		return false, nil
+	}
+
+	if tag, present := r.tagsByImageName[parsed.BaseName]; present {
+		if parsed.FullyQualified {
+			if tag == image {
+				r.found[parsed.BaseName] = true
+			}
+		} else {
+			r.found[parsed.BaseName] = true
+			return true, tag
+		}
+	}
+
+	return false, nil
+}
+
+func (r *imageReplacer) Check() {
+	for imageName := range r.tagsByImageName {
+		if !r.found[imageName] {
+			warner.Warnf("image [%s] is not used by the deployment", imageName)
+		}
+	}
+}

pkg/skaffold/deploy/kubectl/manifests_test.go → pkg/skaffold/deploy/kubectl/images_test.go

@@ -54,6 +54,7 @@ spec:
     name: digest
   - image: skaffold/usedbyfqn:TAG
   - image: skaffold/usedwrongfqn:OTHER
+  - image: in valid
 `)}
 
 	builds := []build.Artifact{{
@@ -92,6 +93,7 @@ spec:
     name: digest
   - image: skaffold/usedbyfqn:TAG
   - image: skaffold/usedwrongfqn:OTHER
+  - image: in valid
 `)}
 
 	defer func(w Warner) { warner = w }(warner)
@@ -102,6 +104,7 @@ spec:
 
 	testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String())
 	testutil.CheckErrorAndDeepEqual(t, false, err, []string{
+		"Couldn't parse image: in valid",
 		"image [skaffold/unused] is not used by the deployment",
 		"image [skaffold/usedwrongfqn] is not used by the deployment",
 	}, fakeWarner.warnings)

pkg/skaffold/deploy/kubectl/manifests.go

@@ -20,17 +20,8 @@ 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
 
@@ -79,84 +70,3 @@ func (l *ManifestList) Diff(latest ManifestList) ManifestList {
 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
-				}
-			}
-		}
-	}
-}

pkg/skaffold/deploy/kubectl/visitor.go

@@ -0,0 +1,79 @@
+/*
+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 (
+	"github.com/pkg/errors"
+	"gopkg.in/yaml.v2"
+)
+
+// Replacer is used to replace portions of yaml manifests that match a given key.
+type Replacer interface {
+	Matches(key string) bool
+
+	NewValue(key string, old interface{}) (bool, interface{})
+}
+
+// Visit recursively visits a list of manifests and applies transformations of them.
+func (l *ManifestList) Visit(replacer Replacer) (ManifestList, error) {
+	var updated 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
+		}
+
+		recursiveVisit(m, replacer)
+
+		updatedManifest, err := yaml.Marshal(m)
+		if err != nil {
+			return nil, errors.Wrap(err, "marshalling yaml")
+		}
+
+		updated = append(updated, updatedManifest)
+	}
+
+	return updated, nil
+}
+
+func recursiveVisit(i interface{}, replacer Replacer) {
+	switch t := i.(type) {
+	case []interface{}:
+		for _, v := range t {
+			recursiveVisit(v, replacer)
+		}
+	case map[interface{}]interface{}:
+		for k, v := range t {
+			key := k.(string)
+
+			if !replacer.Matches(key) {
+				recursiveVisit(v, replacer)
+				continue
+			}
+
+			ok, newValue := replacer.NewValue(key, v)
+			if ok {
+				t[k] = newValue
+			}
+		}
+	}
+}