fix: set kubelet config for containerd in templates #1785
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1785 +/- ##
==========================================
- Coverage 76.39% 76.38% -0.01%
==========================================
Files 130 130
Lines 19351 19356 +5
==========================================
+ Hits 14783 14785 +2
- Misses 3771 3774 +3
Partials 797 797 |
| @@ -1733,6 +1733,11 @@ func (k *KubernetesConfig) GetOrderedKubeletConfigStringForPowershell() string { | |||
| return strings.TrimSuffix(buf.String(), ", ") | |||
| } | |||
|
|
|||
| // HasContainerd returns whether or not we need the containerd runtime | |||
| func (k *KubernetesConfig) HasContainerd() bool { | |||
| return k.ContainerRuntime == KataContainers || k.ContainerRuntime == Containerd | |||
There was a problem hiding this comment.
this could be misleading in the future if we assume this function to mean k.ContainerRuntime == Containerd, now I understand your original naming needsContainerd... Maybe we can make it explicit in the comment that Kata needs containerd as well so it shows in the function info when hovering from different files, wdyt? Also so we don't think later that this is an unintentional bug...
There was a problem hiding this comment.
other than that lgtm
There was a problem hiding this comment.
I think needsContainerd actually is the best option here, with an appropriate comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, jackfrancis 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 |
Reason for Change:
Simplify containerd kubelet config implementation: do this during cloud-init template generation instead of via sed at the last mile.
Also remove some deprecated "if less than k8s 1.8" logic to make the above more maintainable/expressive.
Issue Fixed:
Fixes #1786
Requirements:
Notes: