-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kola/kubeadm: fix CNI selection #205
Conversation
@@ -76,12 +76,13 @@ systemd: | |||
|
|||
func init() { | |||
for _, CNI := range CNIs { | |||
cni := CNI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now that this is variable capturing in the Run
function that bit us here. Nasty. Would a comment about the fact be helpful here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for sure. Actually, this section will be replaced by a new one from this PR (#196) - and a comment is already there to explain while we need such a thing (see https://github.com/kinvolk/mantle/pull/196/files#diff-833c725698d5fe10161a1c491a560567f1473d7541658c722022bc45db2d4c3fR88-R95)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, cool. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. You probably want to squash the fixup commit though. Unless github does it for you when merging.
register action was keeping the latest occurence of `CNI` so it was basically always testing the `cilium` CNI. ... it happens. Signed-off-by: Mathieu Tortuyaux <mathieu@kinvolk.io>
3308011
to
f819ffe
Compare
@krnowak yes.. Github does not handle yet the |
register action was keeping the latest occurence of
CNI
so it wasbasically always testing the
cilium
CNI.... it happens.
The issue was already being fixed into #196 but it's better to be merged now to confirm that flatcar-archive/coreos-overlay#1181 is solving the actual
flannel
issue raised in #196 (comment)