Patchtransformers - drop copied code, improve deletion handling. #3291
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: monopole The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| // ApplySmPatch applies a strategic-merge patch to the | ||
| // selected set of resources. | ||
| ApplySmPatch( | ||
| selectedSet *resource.IdSet, patch *resource.Resource) error |
There was a problem hiding this comment.
Why do we put this method in resmap?
There was a problem hiding this comment.
Before this PR, the code in the body of this method (reswrangler.ApplySmPatch) lives in both PatchStrategicMergeTransformer and PatchTransformer, and has a bug in both places.
The bug is the test target.IsEmpty, followed by the call to target.CurId. This call fails when enable_kyaml is true, because with an RNode backing, there's isn't an ID at all.
We could fix the code in both places and hope it is maintained in both places, or move it to one place, and fix it there.
The fact that the code operates on a resmap (the loop over m.Resources()) suggests adding the code to resmap. I suppose we could push it down into a filter and have it operate on []RNode, but i'm trying to get enable_kyaml working first before doing futher code cleanup.
There was a problem hiding this comment.
This PR should include more tests for reswrangler, to confirm the behavior of delete.
I'll do that.
The actual patch code is covered by all the new tests in resource_test.go this PR adds. They were adapted from the plugin tests.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
No description provided.