-
Notifications
You must be signed in to change notification settings - Fork 242
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
CoreOS host architecture detection fix #882
Conversation
Signed-off-by: Artiom Diomin <[email protected]>
Signed-off-by: Artiom Diomin <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kron4eg 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 |
Signed-off-by: Artiom Diomin <[email protected]>
func genCluster() kubeone.KubeOneCluster { | ||
return kubeone.KubeOneCluster{ | ||
Versions: kubeone.VersionConfig{ | ||
Kubernetes: "v1.17.4", |
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.
Kubernetes: "v1.17.4", | |
Kubernetes: "1.17.4", |
The leading v
in the version generates invalid scripts. Do we have some sort of validation or do we drop the leading v
if the user provides it?
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.
kubeone.KubeOneCluster is validated and defaulted of course, long before it get to this point.
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've tested this and if the user provides a config file such as the following one, provisioning would fail.
apiVersion: kubeone.io/v1alpha1
kind: KubeOneCluster
versions:
kubernetes: "v1.18.2"
cloudProvider:
name: aws
We should add a validation rule or defaulting to default to the version without the leading v
. If you prefer, we can do it in a follow-up.
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.
heh, yeah, v, err := semver.NewVersion(version.Kubernetes)
will parse with v
or without successfully.
Signed-off-by: Artiom Diomin <[email protected]>
/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.
/lgtm
LGTM label has been added. Git tree hash: 02eb08228b84c4237214f29702e7174e9b37409a
|
xref #870 |
What this PR does / why we need it:
Ported fixes for coreOS from #795
Plus golden tests