-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
kubeadm: modify SetKubernetesVersion #69645
kubeadm: modify SetKubernetesVersion #69645
Conversation
05970c2
to
af105bb
Compare
found issue with the initial patch that i sent, applied modification and renamed the PR too. |
af105bb
to
d89ed39
Compare
/retest |
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.
@neolit123 Agree with your idea 👍 EDIT: I misread the PR body. You're correct on the condition change.
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.
Just one concern, otherwise LGTM. Thanks for fixing this! @neolit123
cmd/kubeadm/app/cmd/phases/util.go
Outdated
func SetKubernetesVersion(client clientset.Interface, cfg *kubeadmapiv1beta1.InitConfiguration) error { | ||
if cfg.KubernetesVersion != "" { | ||
|
||
if cfg.KubernetesVersion != kubeadmapiv1beta1.DefaultKubernetesVersion { |
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.
Although cfg.KubernetesVersion
is not empty by default, do we still want to check against the empty case here ? In case it changes to an empty default value for any reason in the future?
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.
/lgtm
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.
hm, so if the default changes to ""
the branch will handle it and return.
but i'm wondering if we should handle ""
with the current default not being ""
.
if cfg.KubernetesVersion != kubeadmapiv1beta1.DefaultKubernetesVersion && cfg.KubernetesVersion != "") {
return nil
}
^ i assume this is what we want?
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.
So, my general hot-take is that we should test all the different scenarios of input to the function.
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.
sounds good.
will update the PR tomorrow.
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.
SGTM :)
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.
This points to a gap in testing, could you please add a unit test for the different scenarios.
cmd/kubeadm/app/cmd/phases/util.go
Outdated
func SetKubernetesVersion(client clientset.Interface, cfg *kubeadmapiv1beta1.InitConfiguration) error { | ||
if cfg.KubernetesVersion != "" { | ||
|
||
if cfg.KubernetesVersion != kubeadmapiv1beta1.DefaultKubernetesVersion { |
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.
So, my general hot-take is that we should test all the different scenarios of input to the function.
d89ed39
to
7c8ce3d
Compare
updated:
|
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.
Good refactoring 👍 Just one concern, otherwise LGTM.
output: ver, | ||
}, | ||
{ | ||
name: "any other version is processed", |
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.
Seems like the logic of SetKubernetesVersion
is "empty version and default version is processed and any other version is skipped". @neolit123 WDYT?
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.
sorry, was in a rush and got things mixed up. i will update the PR later today.
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.
updated. :)
Without this patch SetKubernetesVersion does not set a version in all locations where it's called, because the passed config to it always has a default version stable-1. This always triggers the != "" check and the function returns without setting a version. Validate against DefaultKubernetesVersion and "" instead. This fixes all cases where fetching a version from the internet is not needed at all - e.g. "kubeadm token create". Also make SetKubernetesVersion default to version.Get().String() and add unit tests for the function.
7c8ce3d
to
fa9940c
Compare
/retest |
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.
Thanks for the cleanup @neolit123
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, timothysc 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 |
What this PR does / why we need it:
Without this patch SetKubernetesVersion does not set
a version in all locations where it's called, because
the passed config to it always has a default version
stable-1.
This always triggers the != "" check and the function
returns without setting a version.
Validate against DefaultKubernetesVersion instead.
This fixes all cases where fetching a version from the internet
is not needed at all - e.g. "kubeadm token create".
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):refs #kubernetes/kubeadm#1166
Special notes for your reviewer:
NONE
Release note:
/assign @xiangpengzhao
/assign @fabriziopandini
/cc @timothysc
/kind bug
Peter, this is related to your patch about:
5cf9291
WDYT?
i don't see a problem with having the function like that in the current codebase - i.e it would override with the client version, but this will avoid fetching from the internet if labels are passed.
also, it's not a critical bug, but i'd vote for us to cherry pick this for 1.12.