Minor changes to kubectl and kustomize deployers (#2537)

* Remove duplication

Signed-off-by: David Gageot <david@gageot.net>

* Make kubectl and kustomise deployers look more the same

Signed-off-by: David Gageot <david@gageot.net>

* Add missing deploy failure

Signed-off-by: David Gageot <david@gageot.net>

pkg/skaffold/deploy/kubectl.go

@@ -65,16 +65,6 @@ func (k *KubectlDeployer) Labels() map[string]string {
 	}
 }
 
-type ManifestTransform func(l kubectl.ManifestList, builds []build.Artifact, insecureRegistries map[string]bool) (kubectl.ManifestList, error)
-
-// Transforms are applied to manifests
-var manifestTransforms []ManifestTransform
-
-// AddManifestTransform adds a transform to be applied when deploying.
-func AddManifestTransform(newTransform ManifestTransform) {
-	manifestTransforms = append(manifestTransforms, newTransform)
-}
-
 // 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, labellers []Labeller) error {
@@ -83,8 +73,6 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu
 		color.Default.Fprintln(out, err)
 	}
 
-	event.DeployInProgress()
-
 	manifests, err := k.readManifests(ctx)
 	if err != nil {
 		event.DeployFailed(err)
@@ -95,6 +83,8 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu
 		return nil
 	}
 
+	event.DeployInProgress()
+
 	manifests, err = manifests.ReplaceImages(builds, k.defaultRepo)
 	if err != nil {
 		event.DeployFailed(err)
@@ -110,18 +100,18 @@ func (k *KubectlDeployer) Deploy(ctx context.Context, out io.Writer, builds []bu
 	for _, transform := range manifestTransforms {
 		manifests, err = transform(manifests, builds, k.insecureRegistries)
 		if err != nil {
+			event.DeployFailed(err)
 			return errors.Wrap(err, "unable to transform manifests")
 		}
 	}
 
-	err = k.kubectl.Apply(ctx, out, manifests)
-	if err != nil {
+	if err := k.kubectl.Apply(ctx, out, manifests); err != nil {
 		event.DeployFailed(err)
 		return errors.Wrap(err, "kubectl error")
 	}
 
 	event.DeployComplete()
-	return err
+	return nil
 }
 
 // Cleanup deletes what was deployed by calling Deploy.

pkg/skaffold/deploy/kustomize.go

@@ -126,12 +126,12 @@ func (k *KustomizeDeployer) Deploy(ctx context.Context, out io.Writer, builds []
 	for _, transform := range manifestTransforms {
 		manifests, err = transform(manifests, builds, k.insecureRegistries)
 		if err != nil {
+			event.DeployFailed(err)
 			return errors.Wrap(err, "unable to transform manifests")
 		}
 	}
 
-	err = k.kubectl.Apply(ctx, out, manifests)
-	if err != nil {
+	if err := k.kubectl.Apply(ctx, out, manifests); err != nil {
 		event.DeployFailed(err)
 		return errors.Wrap(err, "kubectl error")
 	}
@@ -154,6 +154,11 @@ func (k *KustomizeDeployer) Cleanup(ctx context.Context, out io.Writer) error {
 	return nil
 }
 
+// Dependencies lists all the files that can change what needs to be deployed.
+func (k *KustomizeDeployer) Dependencies() ([]string, error) {
+	return dependenciesForKustomization(k.KustomizePath)
+}
+
 func dependenciesForKustomization(dir string) ([]string, error) {
 	var deps []string
 
@@ -196,32 +201,22 @@ func dependenciesForKustomization(dir string) ([]string, error) {
 		}
 	}
 
-	deps = append(deps, joinPaths(dir, content.Patches)...)
-	deps = append(deps, joinPaths(dir, content.PatchesStrategicMerge)...)
-	deps = append(deps, joinPaths(dir, content.CRDs)...)
+	deps = append(deps, util.AbsolutePaths(dir, content.Patches)...)
+	deps = append(deps, util.AbsolutePaths(dir, content.PatchesStrategicMerge)...)
+	deps = append(deps, util.AbsolutePaths(dir, content.CRDs)...)
 	for _, patch := range content.PatchesJSON6902 {
 		deps = append(deps, filepath.Join(dir, patch.Path))
 	}
 	for _, generator := range content.ConfigMapGenerator {
-		deps = append(deps, joinPaths(dir, generator.Files)...)
+		deps = append(deps, util.AbsolutePaths(dir, generator.Files)...)
 	}
 	for _, generator := range content.SecretGenerator {
-		deps = append(deps, joinPaths(dir, generator.Files)...)
+		deps = append(deps, util.AbsolutePaths(dir, generator.Files)...)
 	}
 
 	return deps, nil
 }
 
-func joinPaths(root string, paths []string) []string {
-	var list []string
-
-	for _, path := range paths {
-		list = append(list, filepath.Join(root, path))
-	}
-
-	return list
-}
-
 // A kustomization config must be at the root of the direectory. Kustomize will
 // error if more than one of these files exists so order doesn't matter.
 func findKustomizationConfig(dir string) (string, error) {
@@ -245,11 +240,6 @@ func pathExistsLocally(filename string, workingDir string) (bool, os.FileMode) {
 	return false, 0
 }
 
-// Dependencies lists all the files that can change what needs to be deployed.
-func (k *KustomizeDeployer) Dependencies() ([]string, error) {
-	return dependenciesForKustomization(k.KustomizePath)
-}
-
 func (k *KustomizeDeployer) readManifests(ctx context.Context) (kubectl.ManifestList, error) {
 	cmd := exec.CommandContext(ctx, "kustomize", "build", k.KustomizePath)
 	out, err := util.RunCmdOut(cmd)

pkg/skaffold/deploy/transformations.go

@@ -0,0 +1,32 @@
+/*
+Copyright 2019 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 deploy
+
+import (
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/kubectl"
+)
+
+type ManifestTransform func(l kubectl.ManifestList, builds []build.Artifact, insecureRegistries map[string]bool) (kubectl.ManifestList, error)
+
+// Transforms are applied to manifests
+var manifestTransforms []ManifestTransform
+
+// AddManifestTransform adds a transform to be applied when deploying.
+func AddManifestTransform(newTransform ManifestTransform) {
+	manifestTransforms = append(manifestTransforms, newTransform)
+}

pkg/skaffold/util/util.go

@@ -260,15 +260,16 @@ func CloneThroughYAML(old interface{}, new interface{}) error {
 
 // AbsolutePaths prepends each path in paths with workspace if the path isn't absolute
 func AbsolutePaths(workspace string, paths []string) []string {
-	var p []string
+	var list []string
+
 	for _, path := range paths {
-		// TODO(dgageot): this is only done for jib builder.
 		if !filepath.IsAbs(path) {
 			path = filepath.Join(workspace, path)
 		}
-		p = append(p, path)
+		list = append(list, path)
 	}
-	return p
+
+	return list
 }
 
 // IsHiddenDir returns if a directory is hidden.