diff --git a/api/builtins/PatchStrategicMergeTransformer.go b/api/builtins/PatchStrategicMergeTransformer.go index 77ebbc8ae..1f53c5dab 100644 --- a/api/builtins/PatchStrategicMergeTransformer.go +++ b/api/builtins/PatchStrategicMergeTransformer.go @@ -28,45 +28,57 @@ func (p *PatchStrategicMergeTransformerPlugin) Config( return fmt.Errorf("empty file path and empty patch content") } if len(p.Paths) != 0 { - for _, onePath := range p.Paths { - // The following oddly attempts to interpret a path string as an - // actual patch (instead of as a path to a file containing a patch). - // All tests pass if this code is commented out. This code should - // be deleted; the user should use the Patches field which - // exists for this purpose (inline patch declaration). - res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) - if err == nil { - p.loadedPatches = append(p.loadedPatches, res...) - continue - } - res, err = h.ResmapFactory().RF().SliceFromPatches( - h.Loader(), []types.PatchStrategicMerge{onePath}) - if err != nil { - return err - } - p.loadedPatches = append(p.loadedPatches, res...) - } - } - if p.Patches != "" { - res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) + patches, err := loadFromPaths(h, p.Paths) if err != nil { return err } - p.loadedPatches = append(p.loadedPatches, res...) + p.loadedPatches = append(p.loadedPatches, patches...) + } + if p.Patches != "" { + patches, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) + if err != nil { + return err + } + p.loadedPatches = append(p.loadedPatches, patches...) } - if len(p.loadedPatches) == 0 { return fmt.Errorf( "patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches) } - // Merge the patches, looking for conflicts. - _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches) - if err != nil { - return err - } + // TODO(#3723): Delete conflict detection. + // Since #1500 closed, the conflict detector in use doesn't do + // anything useful. The resmap returned by this method hasn't + // been used for many releases. Leaving code as a comment to + // aid in deletion (fixing #3723). + // _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches) + // if err != nil { + // return err + // } return nil } +func loadFromPaths( + h *resmap.PluginHelpers, + paths []types.PatchStrategicMerge) ( + result []*resource.Resource, err error) { + var patches []*resource.Resource + for _, path := range paths { + // For legacy reasons, attempt to treat the path string as + // actual patch content. + patches, err = h.ResmapFactory().RF().SliceFromBytes([]byte(path)) + if err != nil { + // Failing that, treat it as a file path. + patches, err = h.ResmapFactory().RF().SliceFromPatches( + h.Loader(), []types.PatchStrategicMerge{path}) + if err != nil { + return + } + } + result = append(result, patches...) + } + return +} + func (p *PatchStrategicMergeTransformerPlugin) Transform(m resmap.ResMap) error { for _, patch := range p.loadedPatches { target, err := m.GetById(patch.OrgId()) diff --git a/api/filters/nameref/nameref.go b/api/filters/nameref/nameref.go index e5a3d2635..a048d97f1 100644 --- a/api/filters/nameref/nameref.go +++ b/api/filters/nameref/nameref.go @@ -1,7 +1,6 @@ package nameref import ( - "encoding/json" "fmt" "strings" @@ -11,7 +10,6 @@ import ( "sigs.k8s.io/kustomize/api/resmap" "sigs.k8s.io/kustomize/api/resource" "sigs.k8s.io/kustomize/api/types" - "sigs.k8s.io/kustomize/kyaml/filtersutil" "sigs.k8s.io/kustomize/kyaml/kio" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -186,11 +184,7 @@ func (f Filter) recordTheReferral(referral *resource.Resource) { // getRoleRefGvk returns a Gvk in the roleRef field. Return error // if the roleRef, roleRef/apiGroup or roleRef/kind is missing. -func getRoleRefGvk(res json.Marshaler) (*resid.Gvk, error) { - n, err := filtersutil.GetRNode(res) - if err != nil { - return nil, err - } +func getRoleRefGvk(n *yaml.RNode) (*resid.Gvk, error) { roleRef, err := n.Pipe(yaml.Lookup("roleRef")) if err != nil { return nil, err @@ -276,7 +270,7 @@ func (f Filter) roleRefFilter() sieveFunc { if !strings.HasSuffix(f.NameFieldToUpdate.Path, "roleRef/name") { return acceptAll } - roleRefGvk, err := getRoleRefGvk(f.Referrer) + roleRefGvk, err := getRoleRefGvk(f.Referrer.AsRNode()) if err != nil { return acceptAll } diff --git a/api/hasher/hasher_test.go b/api/hasher/hasher_test.go index 594cb6912..0235d409e 100644 --- a/api/hasher/hasher_test.go +++ b/api/hasher/hasher_test.go @@ -56,7 +56,7 @@ kind: ConfigMap`, "6ct58987ht", ""}, {"one key", ` apiVersion: v1 kind: ConfigMap -data: +data: one: ""`, "9g67k2htb6", ""}, // three keys (tests sorting order) {"three keys", ` @@ -224,7 +224,7 @@ kind: ConfigMap`, `{"data":"","kind":"ConfigMap","name":""}`, ""}, {"one key", ` apiVersion: v1 kind: ConfigMap -data: +data: one: ""`, `{"data":{"one":""},"kind":"ConfigMap","name":""}`, ""}, // three keys (tests sorting order) {"three keys", ` diff --git a/api/krusty/multiplepatch_test.go b/api/krusty/multiplepatch_test.go index 37c9ccd43..fa8b321a3 100644 --- a/api/krusty/multiplepatch_test.go +++ b/api/krusty/multiplepatch_test.go @@ -235,6 +235,7 @@ metadata: `) } +// Goal is to remove " emptyDir: {}" with a patch. func TestRemoveEmptyDirWithPatchesAtSameLevel(t *testing.T) { th := kusttest_test.MakeHarness(t) th.WriteK("base", ` @@ -299,8 +300,6 @@ spec: opts := th.MakeDefaultOptions() m := th.Run("overlay", opts) th.AssertActualEqualsExpected( - // TODO(#3394): Should be possible to delete emptyDir with a patch. - // TODO(#3304): DECISION - still a bug, emptyDir should be deleted. m, ` apiVersion: apps/v1 kind: Deployment @@ -318,8 +317,7 @@ spec: - image: sidecar:latest name: sidecar volumes: - - emptyDir: {} - gcePersistentDisk: + - gcePersistentDisk: pdName: nginx-persistent-storage name: nginx-persistent-storage `) @@ -870,25 +868,23 @@ spec: - $patch: delete name: sidecar ` - cases := []struct { - name string + cases := map[string]struct { patch1 string patch2 string expectError bool }{ - { - name: "Patch with delete directive first", + "Patch with delete directive first": { patch1: deletePatch, patch2: additivePatch, }, - { - name: "Patch with delete directive second", + "Patch with delete directive second": { patch1: additivePatch, patch2: deletePatch, }, } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { + for name := range cases { + c := cases[name] + t.Run(name, func(t *testing.T) { th := kusttest_test.MakeHarness(t) makeCommonFilesForMultiplePatchTests(th) th.WriteF("overlay/staging/deployment-patch1.yaml", c.patch1) diff --git a/api/resource/resource.go b/api/resource/resource.go index ba5e579d5..b126b4e92 100644 --- a/api/resource/resource.go +++ b/api/resource/resource.go @@ -15,7 +15,6 @@ import ( "sigs.k8s.io/kustomize/api/konfig" "sigs.k8s.io/kustomize/api/resid" "sigs.k8s.io/kustomize/api/types" - "sigs.k8s.io/kustomize/kyaml/filtersutil" "sigs.k8s.io/kustomize/kyaml/kio" kyaml "sigs.k8s.io/kustomize/kyaml/yaml" "sigs.k8s.io/yaml" @@ -545,18 +544,13 @@ func (r *Resource) AppendRefVarName(variable types.Var) { // ApplySmPatch applies the provided strategic merge patch. func (r *Resource) ApplySmPatch(patch *Resource) error { - node, err := filtersutil.GetRNode(patch) - if err != nil { - return err - } n, ns, k := r.GetName(), r.GetNamespace(), r.GetKind() if patch.NameChangeAllowed() || patch.KindChangeAllowed() { r.StorePreviousId() } - err = r.ApplyFilter(patchstrategicmerge.Filter{ - Patch: node, - }) - if err != nil { + if err := r.ApplyFilter(patchstrategicmerge.Filter{ + Patch: patch.node, + }); err != nil { return err } if r.IsEmpty() { diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go index 7a37c24ce..6dfce8ae2 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go @@ -32,45 +32,57 @@ func (p *plugin) Config( return fmt.Errorf("empty file path and empty patch content") } if len(p.Paths) != 0 { - for _, onePath := range p.Paths { - // The following oddly attempts to interpret a path string as an - // actual patch (instead of as a path to a file containing a patch). - // All tests pass if this code is commented out. This code should - // be deleted; the user should use the Patches field which - // exists for this purpose (inline patch declaration). - res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) - if err == nil { - p.loadedPatches = append(p.loadedPatches, res...) - continue - } - res, err = h.ResmapFactory().RF().SliceFromPatches( - h.Loader(), []types.PatchStrategicMerge{onePath}) - if err != nil { - return err - } - p.loadedPatches = append(p.loadedPatches, res...) - } - } - if p.Patches != "" { - res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) + patches, err := loadFromPaths(h, p.Paths) if err != nil { return err } - p.loadedPatches = append(p.loadedPatches, res...) + p.loadedPatches = append(p.loadedPatches, patches...) + } + if p.Patches != "" { + patches, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) + if err != nil { + return err + } + p.loadedPatches = append(p.loadedPatches, patches...) } - if len(p.loadedPatches) == 0 { return fmt.Errorf( "patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches) } - // Merge the patches, looking for conflicts. - _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches) - if err != nil { - return err - } + // TODO(#3723): Delete conflict detection. + // Since #1500 closed, the conflict detector in use doesn't do + // anything useful. The resmap returned by this method hasn't + // been used for many releases. Leaving code as a comment to + // aid in deletion (fixing #3723). + // _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches) + // if err != nil { + // return err + // } return nil } +func loadFromPaths( + h *resmap.PluginHelpers, + paths []types.PatchStrategicMerge) ( + result []*resource.Resource, err error) { + var patches []*resource.Resource + for _, path := range paths { + // For legacy reasons, attempt to treat the path string as + // actual patch content. + patches, err = h.ResmapFactory().RF().SliceFromBytes([]byte(path)) + if err != nil { + // Failing that, treat it as a file path. + patches, err = h.ResmapFactory().RF().SliceFromPatches( + h.Loader(), []types.PatchStrategicMerge{path}) + if err != nil { + return + } + } + result = append(result, patches...) + } + return +} + func (p *plugin) Transform(m resmap.ResMap) error { for _, patch := range p.loadedPatches { target, err := m.GetById(patch.OrgId()) diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go index 7f9a229dc..c75fc0167 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go @@ -562,6 +562,7 @@ func TestStrategicMergeTransformerNoSchemaMultiPatchesNoConflict(t *testing.T) { th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("PatchStrategicMergeTransformer") defer th.Reset() + // This patch wants to delete "B". th.WriteF("patch1.yaml", ` apiVersion: example.com/v1 kind: Foo @@ -603,10 +604,6 @@ spec: `) assert.NoError(t, err) th.AssertActualEqualsExpectedNoIdAnnotations( - // 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" resMap, ` apiVersion: example.com/v1 kind: Foo @@ -615,7 +612,6 @@ metadata: spec: bar: A: X - B: "Y" C: Z D: W baz: @@ -639,8 +635,7 @@ spec: `) assert.NoError(t, err) th.AssertActualEqualsExpectedNoIdAnnotations( - // This time only patch2 was applied. Same answer on the kyaml - // path, but different answer on apimachinery path (B becomes "true"?) + // This time only patch2 is applied. resMap, ` apiVersion: example.com/v1 kind: Foo