PatchTransfomer readability mods.

api/builtins/PatchTransformer.go

@@ -5,6 +5,7 @@ package builtins
 
 import (
 	"fmt"
+	"strings"
 
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
@@ -28,20 +29,19 @@ type PatchTransformerPlugin struct {
 }
 
 func (p *PatchTransformerPlugin) Config(
-	h *resmap.PluginHelpers, c []byte) (err error) {
+	h *resmap.PluginHelpers, c []byte) error {
-	err = yaml.Unmarshal(c, p)
+	err := yaml.Unmarshal(c, p)
 	if err != nil {
 		return err
 	}
+	p.Patch = strings.TrimSpace(p.Patch)
 	if p.Patch == "" && p.Path == "" {
-		err = fmt.Errorf(
+		return fmt.Errorf(
 			"must specify one of patch and path in\n%s", string(c))
-		return
 	}
 	if p.Patch != "" && p.Path != "" {
-		err = fmt.Errorf(
+		return fmt.Errorf(
 			"patch and path can't be set at the same time\n%s", string(c))
-		return
 	}
 
 	if p.Path != "" {
@@ -54,22 +54,21 @@ func (p *PatchTransformerPlugin) Config(
 
 	patchSM, errSM := h.ResmapFactory().RF().FromBytes([]byte(p.Patch))
 	patchJson, errJson := jsonPatchFromBytes([]byte(p.Patch))
+	if (errSM == nil && errJson == nil) ||
+		(patchSM != nil && patchJson != nil) {
+		return fmt.Errorf(
+			"illegally qualifies as both an SM and JSON patch: [%v]",
+			p.Patch)
+	}
 	if errSM != nil && errJson != nil {
-		err = fmt.Errorf(
+		return fmt.Errorf(
-			"unable to get either a Strategic Merge Patch or JSON patch 6902 from %s", p.Patch)
+			"unable to parse SM or JSON patch from [%v]", p.Patch)
-		return
 	}
-	if errSM == nil && errJson != nil {
+	if errSM == nil {
 		p.loadedPatch = patchSM
-	}
+	} else {
-	if errJson == nil && errSM != nil {
 		p.decodedPatch = patchJson
 	}
-	if patchSM != nil && patchJson != nil {
-		err = fmt.Errorf(
-			"a patch can't be both a Strategic Merge Patch and JSON patch 6902 %s", p.Patch)
-	}
-
 	return nil
 }
 

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -6,6 +6,7 @@ package main
 
 import (
 	"fmt"
+	"strings"
 
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
@@ -32,20 +33,19 @@ type plugin struct {
 var KustomizePlugin plugin
 
 func (p *plugin) Config(
-	h *resmap.PluginHelpers, c []byte) (err error) {
+	h *resmap.PluginHelpers, c []byte) error {
-	err = yaml.Unmarshal(c, p)
+	err := yaml.Unmarshal(c, p)
 	if err != nil {
 		return err
 	}
+	p.Patch = strings.TrimSpace(p.Patch)
 	if p.Patch == "" && p.Path == "" {
-		err = fmt.Errorf(
+		return fmt.Errorf(
 			"must specify one of patch and path in\n%s", string(c))
-		return
 	}
 	if p.Patch != "" && p.Path != "" {
-		err = fmt.Errorf(
+		return fmt.Errorf(
 			"patch and path can't be set at the same time\n%s", string(c))
-		return
 	}
 
 	if p.Path != "" {
@@ -58,22 +58,21 @@ func (p *plugin) Config(
 
 	patchSM, errSM := h.ResmapFactory().RF().FromBytes([]byte(p.Patch))
 	patchJson, errJson := jsonPatchFromBytes([]byte(p.Patch))
+	if (errSM == nil && errJson == nil) ||
+		(patchSM != nil && patchJson != nil) {
+		return fmt.Errorf(
+			"illegally qualifies as both an SM and JSON patch: [%v]",
+			p.Patch)
+	}
 	if errSM != nil && errJson != nil {
-		err = fmt.Errorf(
+		return fmt.Errorf(
-			"unable to get either a Strategic Merge Patch or JSON patch 6902 from %s", p.Patch)
+			"unable to parse SM or JSON patch from [%v]", p.Patch)
-		return
 	}
-	if errSM == nil && errJson != nil {
+	if errSM == nil {
 		p.loadedPatch = patchSM
-	}
+	} else {
-	if errJson == nil && errSM != nil {
 		p.decodedPatch = patchJson
 	}
-	if patchSM != nil && patchJson != nil {
-		err = fmt.Errorf(
-			"a patch can't be both a Strategic Merge Patch and JSON patch 6902 %s", p.Patch)
-	}
-
 	return nil
 }
 

plugin/builtin/patchtransformer/PatchTransformer_test.go

@@ -12,7 +12,7 @@ import (
 )
 
 const (
-	target = `
+	someDeploymentResources = `
 apiVersion: apps/v1
 metadata:
   name: myDeploy
@@ -76,7 +76,7 @@ kind: PatchTransformer
 metadata:
   name: notImportantHere
 path: patch.yaml
-`, target, func(t *testing.T, err error) {
+`, someDeploymentResources, func(t *testing.T, err error) {
 		if err == nil {
 			t.Fatalf("expected error")
 		}
@@ -98,12 +98,13 @@ kind: PatchTransformer
 metadata:
   name: notImportantHere
 patch: "thisIsNotAPatch"
-`, target, func(t *testing.T, err error) {
+`, someDeploymentResources, func(t *testing.T, err error) {
 		if err == nil {
 			t.Fatalf("expected error")
 		}
+		fmt.Print(err)
 		if !strings.Contains(err.Error(),
-			"unable to get either a Strategic Merge Patch or JSON patch 6902 from") {
+			"unable to parse SM or JSON patch from ") {
 			t.Fatalf("unexpected err: %v", err)
 		}
 	})
@@ -120,7 +121,7 @@ kind: PatchTransformer
 metadata:
   name: notImportantHere
 patch: '[{"op": "add", "path": "/spec/template/spec/dnsPolicy", "value": "ClusterFirst"}]'
-`, target, func(t *testing.T, err error) {
+`, someDeploymentResources, func(t *testing.T, err error) {
 		if err == nil {
 			t.Fatalf("expected error")
 		}
@@ -131,6 +132,30 @@ patch: '[{"op": "add", "path": "/spec/template/spec/dnsPolicy", "value": "Cluste
 	})
 }
 
+func TestPatchTransformerBlankPatch(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarness(t).
+		PrepBuiltin("PatchTransformer")
+	defer th.Reset()
+
+	th.RunTransformerAndCheckError(`
+apiVersion: builtin
+kind: PatchTransformer
+metadata:
+  name: notImportantHere
+patch: "  "
+target:
+  name: .*Deploy
+`, someDeploymentResources, func(t *testing.T, err error) {
+		if err == nil {
+			t.Fatalf("expected error")
+		}
+		if !strings.Contains(err.Error(),
+			"must specify one of patch and path in") {
+			t.Fatalf("unexpected err: %v", err)
+		}
+	})
+}
+
 func TestPatchTransformerBothEmptyPathAndPatch(t *testing.T) {
 	th := kusttest_test.MakeEnhancedHarness(t).
 		PrepBuiltin("PatchTransformer")
@@ -141,11 +166,12 @@ apiVersion: builtin
 kind: PatchTransformer
 metadata:
   name: notImportantHere
-`, target, func(t *testing.T, err error) {
+`, someDeploymentResources, func(t *testing.T, err error) {
 		if err == nil {
 			t.Fatalf("expected error")
 		}
-		if !strings.Contains(err.Error(), "must specify one of patch and path in") {
+		if !strings.Contains(err.Error(),
+			"must specify one of patch and path in") {
 			t.Fatalf("unexpected err: %v", err)
 		}
 	})
@@ -163,11 +189,12 @@ metadata:
   name: notImportantHere
 Path: patch.yaml
 Patch: "something"
-`, target, func(t *testing.T, err error) {
+`, someDeploymentResources, func(t *testing.T, err error) {
 		if err == nil {
 			t.Fatalf("expected error")
 		}
-		if !strings.Contains(err.Error(), "patch and path can't be set at the same time") {
+		if !strings.Contains(err.Error(),
+			"patch and path can't be set at the same time") {
 			t.Fatalf("unexpected err: %v", err)
 		}
 	})
@@ -196,7 +223,7 @@ path: patch.yaml
 target:
   name: .*Deploy
 `,
-		target,
+		someDeploymentResources,
 		`
 apiVersion: apps/v1
 kind: Deployment
@@ -426,7 +453,7 @@ spec:
 			th.WriteF("patch.yaml", patch)
 
 			c := fmt.Sprintf(config, tc.yamlSupport)
-			rm := th.LoadAndRunTransformer(c, target)
+			rm := th.LoadAndRunTransformer(c, someDeploymentResources)
 			th.AssertActualEqualsExpected(rm, tc.expectedOutput)
 		})
 	}
@@ -446,7 +473,7 @@ patch: '[{"op": "replace", "path": "/spec/template/spec/containers/0/image", "va
 target:
   name: .*Deploy
   kind: Deployment
-`, target,
+`, someDeploymentResources,
 		`
 apiVersion: apps/v1
 kind: Deployment
@@ -520,7 +547,7 @@ patch: |-
   kind: Deployment
   spec:
     replica: 77
-`, target, `
+`, someDeploymentResources, `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -573,7 +600,7 @@ spec:
 `)
 }
 
-const ingressTarget = `apiVersion: networking.k8s.io/v1beta1
+const anIngressResource = `apiVersion: networking.k8s.io/v1beta1
 kind: Ingress
 metadata:
   name: my-ingress
@@ -623,7 +650,7 @@ metadata:
 path: patch.json
 target:
   kind: Ingress
-`, ingressTarget, `
+`, anIngressResource, `
 apiVersion: networking.k8s.io/v1beta1
 kind: Ingress
 metadata:
@@ -673,7 +700,7 @@ metadata:
 path: patch.yaml
 target:
   kind: Ingress
-`, ingressTarget, `
+`, anIngressResource, `
 apiVersion: networking.k8s.io/v1beta1
 kind: Ingress
 metadata: