Merge pull request #3315 from monopole/hoserface

Add more tests

api/filters/patchstrategicmerge/patchstrategicmerge_test.go

@@ -15,6 +15,78 @@ func TestFilter(t *testing.T) {
 		patch    *yaml.RNode
 		expected string
 	}{
+		"nullMapEntry1": {
+			input: `
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B:
+    C: Z
+`,
+			patch: yaml.MustParse(`
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`),
+			expected: `
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`,
+		},
+		"nullMapEntry2": {
+			input: `
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`,
+			patch: yaml.MustParse(`
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B:
+    C: Z
+`),
+			expected: `
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`,
+		},
 		"simple patch": {
 			input: `
 apiVersion: apps/v1

api/konfig/general.go

@@ -19,9 +19,30 @@ func DefaultKustomizationFileName() string {
 	return RecognizedKustomizationFileNames()[0]
 }
 
+// IfApiMachineryElseKyaml returns true if executing the apimachinery code
+// path, else we're executing the kyaml code paths.
+func IfApiMachineryElseKyaml(s1, s2 string) string {
+	if !FlagEnableKyamlDefaultValue {
+		return s1
+	}
+	return s2
+}
+
 const (
 	// FlagEnableKyamlDefaultValue is the default value for the --enable_kyaml
 	// flag.  This value is also used in unit tests.  See provider.DepProvider.
+	//
+	// TODO(#3304): eliminate branching on this constant.
+	// Details: https://github.com/kubernetes-sigs/kustomize/issues/3304
+	//
+	// All tests should pass for either true or false values
+	// of this constant, without having to check its value.
+	// In the cases where there's a different outcome, either decide
+	// that the difference is acceptable, or make the difference go away.
+	//
+	// Historically, tests passed for enable_kyaml == false, i.e. using
+	// apimachinery libs.  This doesn't mean the code was better, it just
+	// means regression tests preserved those outcomes.
 	FlagEnableKyamlDefaultValue = false
 
 	// An environment variable to consult for kustomization

api/resmap/factory_test.go

@@ -10,10 +10,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	"sigs.k8s.io/kustomize/api/filesys"
 	"sigs.k8s.io/kustomize/api/ifc"
+	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/kv"
 	"sigs.k8s.io/kustomize/api/loader"
-	"sigs.k8s.io/kustomize/api/resid"
 	. "sigs.k8s.io/kustomize/api/resmap"
+	"sigs.k8s.io/kustomize/api/resource"
 	resmaptest_test "sigs.k8s.io/kustomize/api/testutils/resmaptest"
 	valtest_test "sigs.k8s.io/kustomize/api/testutils/valtest"
 	"sigs.k8s.io/kustomize/api/types"
@@ -110,8 +111,6 @@ metadata:
 	assert.Equal(t, expYaml, mYaml)
 }
 
-var cmap = resid.Gvk{Version: "v1", Kind: "ConfigMap"}
-
 func TestNewFromConfigMaps(t *testing.T) {
 	type testCase struct {
 		description string
@@ -329,3 +328,69 @@ rules:
 		t.Fatalf("error: %s", err)
 	}
 }
+
+func TestMerge_Empty(t *testing.T) {
+	rm, err := rmF.Merge([]*resource.Resource{})
+	assert.NoError(t, err)
+	assert.Equal(t, 0, rm.Size())
+}
+
+func TestMerge(t *testing.T) {
+	var (
+		err    error
+		yml    []byte
+		r1, r2 *resource.Resource
+	)
+	r1, err = rf.FromBytes([]byte(`apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B:
+    C: Z
+`))
+	assert.NoError(t, err)
+
+	r2, err = rf.FromBytes([]byte(`apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`))
+	assert.NoError(t, err)
+
+	rm, err := rmF.Merge([]*resource.Resource{r1, r2})
+	assert.NoError(t, err)
+
+	yml, err = rm.AsYaml()
+	assert.NoError(t, err)
+
+	assert.Equal(t, konfig.IfApiMachineryElseKyaml(`apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B: null
+    C: Z
+    D: W
+  baz:
+    hello: world
+`, `apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`), string(yml))
+}

api/resmap/reswrangler_test.go

@@ -20,8 +20,9 @@ import (
 	"sigs.k8s.io/kustomize/api/types"
 )
 
-var rf = provider.NewDefaultDepProvider().GetResourceFactory()
-var rmF = NewFactory(rf, nil)
+var depProvider = provider.NewDefaultDepProvider()
+var rf = depProvider.GetResourceFactory()
+var rmF = NewFactory(rf, depProvider.GetConflictDetectorFactory())
 
 func doAppend(t *testing.T, w ResMap, r *resource.Resource) {
 	err := w.Append(r)
@@ -192,6 +193,8 @@ metadata:
 }
 
 func TestGetMatchingResourcesByCurrentId(t *testing.T) {
+	cmap := resid.Gvk{Version: "v1", Kind: "ConfigMap"}
+
 	r1 := rf.FromMap(
 		map[string]interface{}{
 			"apiVersion": "v1",
@@ -771,7 +774,7 @@ rules:
 	}
 }
 
-func TestApplySmPatch_Namespaces(t *testing.T) {
+func TestApplySmPatch_General(t *testing.T) {
 	const (
 		myDeployment      = "Deployment"
 		myCRD             = "myCRD"
@@ -801,6 +804,53 @@ spec:
 		errorExpected bool
 		errorMsg      string
 	}{
+		"confusion": {
+			base: []string{`apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    B: Y
+`,
+			},
+			patches: []string{`apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B:
+    C: Z
+`, `apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`,
+			},
+			errorExpected: false,
+			expected: []string{
+				`apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    C: Z
+    D: W
+  baz:
+    hello: world
+`,
+			},
+		},
 		"withschema-ns1-ns2-one": {
 			base: []string{
 				addNamespace("ns1", baseResource(myDeployment)),

api/resource/resource_test.go

@@ -207,6 +207,100 @@ spec:
 `, string(bytes))
 }
 
+func TestApplySmPatch_2(t *testing.T) {
+	resource, err := factory.FromBytes([]byte(`
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    B: Y
+`))
+	assert.NoError(t, err)
+	patch, err := factory.FromBytes([]byte(`
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`))
+	assert.NoError(t, err)
+	assert.NoError(t, resource.ApplySmPatch(patch))
+	bytes, err := resource.AsYAML()
+	assert.NoError(t, err)
+	assert.Equal(t, `apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    C: Z
+    D: W
+  baz:
+    hello: world
+`, string(bytes))
+}
+
+func TestApplySmPatch_SwapOrder(t *testing.T) {
+	s1 := `
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    B:
+    C: Z
+`
+	s2 := `
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`
+	expected := `apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    C: Z
+    D: W
+  baz:
+    hello: world
+`
+	r1, err := factory.FromBytes([]byte(s1))
+	assert.NoError(t, err)
+	r2, err := factory.FromBytes([]byte(s2))
+	assert.NoError(t, err)
+	assert.NoError(t, r1.ApplySmPatch(r2))
+	bytes, err := r1.AsYAML()
+	assert.NoError(t, err)
+	assert.Equal(t, expected, string(bytes))
+
+	r1, _ = factory.FromBytes([]byte(s1))
+	r2, _ = factory.FromBytes([]byte(s2))
+	assert.NoError(t, r2.ApplySmPatch(r1))
+	bytes, err = r2.AsYAML()
+	assert.NoError(t, err)
+	assert.Equal(t, expected, string(bytes))
+}
+
 func TestApplySmPatch(t *testing.T) {
 	const (
 		myDeployment = "Deployment"

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go

@@ -13,20 +13,6 @@ import (
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 )
 
-// TODO(#3304): eliminate branching on konfig.FlagEnableKyamlDefaultValue
-// Details: https://github.com/kubernetes-sigs/kustomize/issues/3304
-// All tests should pass for either true or false values
-// of this boolean, without having to check its value.
-// I.e. fix all the cases where FlagEnableKyamlDefaultValue == true,
-// and delete all reads of the constant.  Historically,
-// the code worked for enable_kyaml == false.
-func ifApiMachineryElseKyaml(s1, s2 string) string {
-	if !konfig.FlagEnableKyamlDefaultValue {
-		return s1
-	}
-	return s2
-}
-
 // TODO(#3304)
 const skipConflictDetectionTests = konfig.FlagEnableKyamlDefaultValue
 
@@ -573,11 +559,10 @@ spec:
 `)
 }
 
-func TestStrategicMergeTransformerNoSchemaMultiPatches(t *testing.T) {
+func TestStrategicMergeTransformerNoSchemaMultiPatchesNoConflict(t *testing.T) {
 	th := kusttest_test.MakeEnhancedHarness(t).
 		PrepBuiltin("PatchStrategicMergeTransformer")
 	defer th.Reset()
-
 	th.WriteF("patch1.yaml", `
 apiVersion: example.com/v1
 kind: Foo
@@ -600,7 +585,7 @@ spec:
   baz:
     hello: world
 `)
-	th.RunTransformerAndCheckResult(`
+	resMap, err := th.RunTransformer(`
 apiVersion: builtin
 kind: PatchStrategicMergeTransformer
 metadata:
@@ -608,9 +593,23 @@ metadata:
 paths:
 - patch1.yaml
 - patch2.yaml
-`,
-		targetNoschema,
-		ifApiMachineryElseKyaml(`
+`, `apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    B: Y
+`)
+	assert.NoError(t, err)
+	th.AssertActualEqualsExpected(
+		resMap,
+		// In kyaml/yaml.merge2, the empty "B: " is dropped
+		// when patch1 and patch2 are merged, so the patch
+		// applied is effectively only patch2.yaml.
+		// So it cannot delete the "B: Y"
+		konfig.IfApiMachineryElseKyaml(`
 apiVersion: example.com/v1
 kind: Foo
 metadata:
@@ -625,6 +624,55 @@ spec:
 `, `
 apiVersion: example.com/v1
 kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    B: "Y"
+    C: Z
+    D: W
+  baz:
+    hello: world
+`))
+	resMap, err = th.RunTransformer(`
+apiVersion: builtin
+kind: PatchStrategicMergeTransformer
+metadata:
+  name: notImportantHere
+paths:
+- patch2.yaml
+`, `apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    B: Y
+`)
+	assert.NoError(t, err)
+	th.AssertActualEqualsExpected(
+		resMap,
+		// This time only patch2 was applied.  Same answer on the kyaml
+		// path, but different answer on apimachinery path (B becomes "true"?)
+		// The kyaml path is doing better here.
+		konfig.IfApiMachineryElseKyaml(`
+apiVersion: example.com/v1
+kind: Foo
+metadata:
+  name: my-foo
+spec:
+  bar:
+    A: X
+    B: true
+    C: Z
+    D: W
+  baz:
+    hello: world
+`, `
+apiVersion: example.com/v1
+kind: Foo
 metadata:
   name: my-foo
 spec: