🌱 Fix empty control-plane taints example #7025
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
@apricote: This issue is currently awaiting triage. If CAPI contributors determines this is a relevant issue, they will accept it by applying the The 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
needs-triage
| @@ -220,7 +220,7 @@ type NodeRegistrationOptions struct { | |||
|
|
|||
| // Taints specifies the taints the Node API object should be registered with. If this field is unset, i.e. nil, in the `kubeadm init` process | |||
| // it will be defaulted to []v1.Taint{'node-role.kubernetes.io/master=""'}. If you don't want to taint your control-plane node, set this field to an | |||
| // empty slice, i.e. `taints: {}` in the YAML file. This field is solely used for Node registration. | |||
| // empty slice, i.e. `taints: []` in the YAML file. This field is solely used for Node registration. | |||
There was a problem hiding this comment.
I was not sure if it is okay to change the upstream kubeadm types, but without changing this one description in the generated CRDs still contained the taints: {} example.
There was a problem hiding this comment.
Let's keep the upstream types as is. We generally try to keep the diffs there minimal and the godoc here is not exposed anywhere to CAPI users.
There was a problem hiding this comment.
This godoc is exposed directly to CAPI users for v1alpha3:
But as @killianmuldoon said, we should probably skip making these changes to out-of-support CRD versions, so I am going to remove this change from the PR.
There was a problem hiding this comment.
Ah good point, you're right.
|
/retest |
There was a problem hiding this comment.
We should IMO only make this change in v1beta1 as v1alpha4 is no longer directly supported.
It looks like this change could also be made to our kubeadm v1beta1 upstream type, but I don't that we need to include that type anymore. It might be safe to drop it as it's used for very old, and I think unsupported clusters.
v1beta1 doesn't even appear on our support matrix. I think it's there to allow potentially updating very old clusters without going through multiple cluster api versions.
@fabriziopandini @sbueringer WDYT?
Should we clean up (remove) some of those files? Or is there a reason to keep them around? |
My understanding is we have to keep those API versions around as long as we support upgrading from an older API version to the current one. I think we're not due to drop support until we drop support for Kubernetes 1.22 (if I'm reading the table right), but it might be dropped sooner or later depending on the community. |
The docs to remove the default control plane taints instruct the user
to set the value to _an empty slice, i.e. `taints: {}` in the YAML file_
, but an empty slice is equal to an empty yaml array: `taints: []`.
k8s-ci-robot
added
size/S
|
I removed all changes to API versions besides |
|
@stmcginnis @killianmuldoon The reason why we currently keep the apiVersions is because while we don't support v1alpha3 CAPI versions anymore we might still want to support upgrading from v1alpha3 to a supported version. Also in my opinion we have to fix the following issue first before we can drop apiVersions: #5470 |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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
What this PR does / why we need it:
The docs to remove the default control plane taints instruct the user to set the value to an empty slice, i.e.
taints: {}in the YAML file , but an empty slice is equal to an empty yaml array:taints: [].Which issue(s) this PR fixes:
Fixes #