From d08b9c30ee34b84f459652e6341a4a8b075a3995 Mon Sep 17 00:00:00 2001 From: jregan Date: Sun, 6 Dec 2020 12:47:03 -0800 Subject: [PATCH] What is this? --- .../patchstrategicmerge_test.go | 72 ++++++++++++++ api/konfig/general.go | 21 +++++ api/resmap/factory_test.go | 71 +++++++++++++- api/resmap/reswrangler_test.go | 56 ++++++++++- api/resource/resource_test.go | 94 +++++++++++++++++++ .../PatchStrategicMergeTransformer_test.go | 88 +++++++++++++---- 6 files changed, 376 insertions(+), 26 deletions(-) diff --git a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go b/api/filters/patchstrategicmerge/patchstrategicmerge_test.go index 81456e9bb..a2b8ab560 100644 --- a/api/filters/patchstrategicmerge/patchstrategicmerge_test.go +++ b/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 diff --git a/api/konfig/general.go b/api/konfig/general.go index 9db3afaad..4325bd99b 100644 --- a/api/konfig/general.go +++ b/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 diff --git a/api/resmap/factory_test.go b/api/resmap/factory_test.go index bd765036a..9e0e1bf2e 100644 --- a/api/resmap/factory_test.go +++ b/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)) +} diff --git a/api/resmap/reswrangler_test.go b/api/resmap/reswrangler_test.go index 86bab5be6..a86357d4c 100644 --- a/api/resmap/reswrangler_test.go +++ b/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)), diff --git a/api/resource/resource_test.go b/api/resource/resource_test.go index ba147e9cc..665e36627 100644 --- a/api/resource/resource_test.go +++ b/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" diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go index 380c9c27d..8af85632b 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go +++ b/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: