fix(kyaml/yaml): minor nil safety fix for RNode.Content etc #5985
Conversation
…l-safe" This change is addressing observed panics within kustomize that obscure the actual failure. The primary observed problem case involves RNode.Content.
|
This PR has multiple commits, and the default merge method is: merge. 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-sigs/prow repository. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joeycumines 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 |
|
Welcome @joeycumines! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @joeycumines. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
|
||
| // Content returns Node Content field. | ||
| func (rn *RNode) Content() []*yaml.Node { | ||
| if rn == nil { |
There was a problem hiding this comment.
Please remain to check if rn == nil
| if rn.YNode().Kind != yaml.MappingNode { | ||
| return nil | ||
| if yNode := rn.YNode(); yNode != nil { | ||
| if yNode.Kind != yaml.MappingNode { |
There was a problem hiding this comment.
Please don't make the indent too deep here.
I think it's better to first store the value in the local variable of yNode and then check with if yNode == nil in an early-return format.
| if rn == nil { | ||
| return nil | ||
| if yNode := rn.YNode(); yNode != nil { | ||
| return yNode.Content |
There was a problem hiding this comment.
Same.
I think it's better to first store the value in the local variable of yNode and then check with if yNode == nil in an early-return format.
| elem, err := rn.Pipe(MatchElement(key, value)) | ||
| if err != nil { | ||
| return nil | ||
| if yNode := rn.YNode(); yNode != nil { |
| elem, err := rn.Pipe(MatchElementList(keys, values)) | ||
| if err != nil { | ||
| return nil | ||
| if yNode := rn.YNode(); yNode != nil { |
| // Both of these scenarios should cause rn.YNode() to return nil. | ||
| nodesToTest := [...]struct { | ||
| name string | ||
| rn *RNode |
There was a problem hiding this comment.
Please add a test case that rn is nil
The
RNode.YNodemethod appears to make an effort to be "nil-safe". This change applies the same sort of behavior to a subset of other methods, e.g.RNode.Content, which access fields on the value returned by that method.N.B. This change was originally prepared based on v5.4.2, as panics were observed when applying patches, using the
kustomizecommand, indirectly, viaskaffold. I didn't go as far as to determine what specifically was happening there, and whatever issue was present is notably resolved, as of v5.7.1.With the above in mind, please close this PR if this change is not relevant or desirable 😄