improve patch roundtripping #376
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder 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
approved
|
FYI @neolit123 @munnerz This should resolve the roundtripping concerns as well, patches should roundtrip properly now where possible |
| for _, node := range in.Nodes { | ||
| convertedNode := Node{} | ||
| if err := Convert_config_Node_To_v1alpha2_Node(&node, &convertedNode, s); err != nil { | ||
| return err | ||
| } | ||
| // the first control plane is treated specially |
There was a problem hiding this comment.
specifically this new snippet means we roundtrip the global patches to v1alpha2
we should get people off of v1alpha2 though and we should not actually roundtrip to it, because per-node patches will NOT roundtrip correctly as they were ignored in v1alpha2
probably, but I want to improve the patch behavior next and avoid breaking existing users 😅 |
|
It'll be great to get some fuzz tests in this area of the codebase at some point. I'd love to see some signal on whether we're actually getting these conversions correct or not before we starting utilising them more seriously in later api versions 😄 /lgtm |
k8s-ci-robot
added
the
lgtm
Follow up to #373
In kind versions we shipped with the v1alpha2 config the kubeadm config patches were only respected if they were on the bootstrap control plane node. this re-introduces that behavior when converting from v1alpha2 to internal.
This allows us to preserve the behavior of existing config files for now, while leaving room to introduce per-node patches that are actually used going forward.