Fix bug with merge2 with null values #5365
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shuheiktgw The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @shuheiktgw. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/M
| @@ -725,7 +725,7 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) { | |||
| } | |||
|
|
|||
| // Clear the field if it is empty, or explicitly null | |||
| if s.Value == nil || s.Value.IsTaggedNull() { | |||
There was a problem hiding this comment.
There are three possible ways to set s.Value.
- Set the
Valuefield directly - Use the SetField function
- Use the Set function
I've checked every use case and found two cases in which s.Value could be null.
So I need to investigate the two cases further to clarify how this change affect them...
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
Related issues: * kubernetes-sigs#5031 * kubernetes-sigs#5171 After noting this behaviour was not present in d89b448 a `git bisect` pointed to the change 1b7db20. The issue with that change is that upon seeing a `null` node it would replace it with a node whose value was equivalent but without a `!!null` tag. This meant that one application of a patch would have the desired approach: the result would be `null` in the output, but on a second application of a similar patch the field would be rendered as `"null"`. To avoid this, define a special flag node that is `null` but never removed during merges. The added `TestApplySmPatch_Idempotency` test verifies this behaviour. However, this approach will may change the value of the node in the output, e.g. if originally there was `field: ~` it would be replaced by `field: null`. The added test case in `kyaml/yaml/merge2/scalar_test.go` demonstrates this behaviour (this test currently fails as I expect the desired outcome is to preserve the null marker) See also kubernetes-sigs#5365 for an alternative approach
Related issues: * kubernetes-sigs#5031 * kubernetes-sigs#5171 After noting this behaviour was not present in d89b448 a `git bisect` pointed to the change 1b7db20. The issue with that change is that upon seeing a `null` node it would replace it with a node whose value was equivalent but without a `!!null` tag. This meant that one application of a patch would have the desired approach: the result would be `null` in the output, but on a second application of a similar patch the field would be rendered as `"null"`. To avoid this, define a special flag node that is `null` but never removed during merges. The added `TestApplySmPatch_Idempotency` test verifies this behaviour. However, this approach will may change the value of the node in the output, e.g. if originally there was `field: ~` it would be replaced by `field: null`. The added test case in `kyaml/yaml/merge2/scalar_test.go` demonstrates this behaviour (this test currently fails as I expect the desired outcome is to preserve the null marker) See also kubernetes-sigs#5365 for an alternative approach
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
Related issues: * kubernetes-sigs#5031 * kubernetes-sigs#5171 After noting this behaviour was not present in d89b448 a `git bisect` pointed to the change 1b7db20. The issue with that change is that upon seeing a `null` node it would replace it with a node whose value was equivalent but without a `!!null` tag. This meant that one application of a patch would have the desired approach: the result would be `null` in the output, but on a second application of a similar patch the field would be rendered as `"null"`. To avoid this, define a new attribute on `RNode`s that is checked before clearing any node we should keep. The added `TestApplySmPatch_Idempotency` test verifies this behaviour. See also kubernetes-sigs#5365 for an alternative approach
Related issues: * kubernetes-sigs#5031 * kubernetes-sigs#5171 After noting this behaviour was not present in d89b448 a `git bisect` pointed to the change 1b7db20. The issue with that change is that upon seeing a `null` node it would replace it with a node whose value was equivalent but without a `!!null` tag. This meant that one application of a patch would have the desired approach: the result would be `null` in the output, but on a second application of a similar patch the field would be rendered as `"null"`. To avoid this, define a new attribute on `RNode`s that is checked before clearing any node we should keep. The added `TestApplySmPatch_Idempotency` test verifies this behaviour. See also kubernetes-sigs#5365 for an alternative approach
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
|
Superseded by #5519 |
|
/close |
|
@stormqueen1990: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Related issues: * kubernetes-sigs#5031 * kubernetes-sigs#5171 After noting this behaviour was not present in d89b448 a `git bisect` pointed to the change 1b7db20. The issue with that change is that upon seeing a `null` node it would replace it with a node whose value was equivalent but without a `!!null` tag. This meant that one application of a patch would have the desired approach: the result would be `null` in the output, but on a second application of a similar patch the field would be rendered as `"null"`. To avoid this, define a new attribute on `RNode`s that is checked before clearing any node we should keep. The added `TestApplySmPatch_Idempotency` test verifies this behaviour. See also kubernetes-sigs#5365 for an alternative approach
Related issues
Fixes #5031
Problem
#4890 originally introduced the problem. The change removes the null tag from the target nodeto preserve the null value, and it works fine if users only merge the files once. However, a problem occurs if users try to merge another file to the output since the node, which used to be tagged as null, does not have any tags attached and looks like a plain ScalarRNode from kyaml's perspective. As a result, the FieldSetter adds DoubleQuotedStyle to the node that used to be tagged as null
, and that leads to #5031.
I've added a test case called TestMerge_null to demonstrate the problem, so please take a look at it as well.
Proposed changes
The original problem was that we could not return null nodes from the Merger#VisitMap since then the FieldSetter#Filter removed them. In this PR, I've changed the FieldSetter#Filter and stopped deleting null nodes. Instead, when we want to remove the nodes, we can simply return walk.ClearNode.