Skip to content
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.

Sign up for GitHub

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

Jump to bottom

kubeadm: Allows to use versions like 1.6.4 instead v1.6.4 #48507

Merged
merged 1 commit into from
Jul 12, 2017
Merged

kubeadm: Allows to use versions like 1.6.4 instead v1.6.4 #48507

merged 1 commit into from
Jul 12, 2017

Conversation

kad
Copy link
Member

@kad kad commented Jul 5, 2017

What this PR does / why we need it:

As part of issue kubernetes/kubeadm#292 discussion, it
turned out that for users it is not always obvious that
version specification parameter must be in form "vX.Y.Z".
This patch allows to specify it in form "X.Y.Z" and
converts it internally to normal semantic version which
expected in the rest of the code.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

Special notes for your reviewer:

Release note:

- kubeadm now can accept versions like "1.6.4" where previously it strictly required "v1.6.4"

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 5, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 5, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @kad. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2017
@luxas luxas assigned timothysc and pipejakob and unassigned mikedanese Jul 5, 2017
@luxas
Copy link
Member

luxas commented Jul 5, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2017
@luxas
Copy link
Member

luxas commented Jul 5, 2017

@mikedanese @pipejakob @timothysc @jbeda @ixdy What do you think of this?
Is it worth it to be tolerant to non-semver versions? Where do we draw the line in that case?

@ixdy
Copy link
Member

ixdy commented Jul 5, 2017

@luxas I don't think the v is part of the semantic version spec. I'm not sure why we add it to our version strings.

@luxas
Copy link
Member

luxas commented Jul 6, 2017

@ixdy Good, point. Then I'm not sure why pkg/util/version.ParseSemantic() is the only func that can handle v...

@kad
Copy link
Member Author

kad commented Jul 6, 2017

Actually versionMatchRE in pkg/util/version will handle both variants with and without v. But in this particular case inside kubeadm we need to anyway transform it to form "vX.Y.Z" as it will be used for tags in registry.

luxas
luxas reviewed Jul 6, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style nit
I guess I'm ok with this then ;)

cmd/kubeadm/app/util/version.go Outdated
@@ -49,7 +49,11 @@ var (
// latest-1.0 (and similarly 1.1, 1.2, 1.3, ...)
func KubernetesReleaseVersion(version string) (string, error) {
if kubeReleaseRegex.MatchString(version) {
return version, nil
if version[0] == 'v' {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about

if strings.HasPrefix(version, "v") { 
return version, nil
}
return fmt.Sprintf("v%s", version), nil

instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first part (HasPrefix) ok, will update. for fmt.Sprintf, I think for such trivial operation, + operator will be a lot faster than Sprintf.

@kad
Allows to use versions like 1.6.4 instead v1.6.4
d8fcf4f 
As part of issue kubernetes/kubeadm#292 discussion, it
turned out that for users it is not always obvious that
version specification parameter must be in form "vX.Y.Z".
This patch allows to specify it in form "X.Y.Z" and
converts it internally to normal semantic version which
expected in the rest of the code.
@kad
Copy link
Member Author

kad commented Jul 6, 2017

flake ? :(

@ixdy
Copy link
Member

ixdy commented Jul 6, 2017

looks like it.
/retest

@luxas
Copy link
Member

luxas commented Jul 6, 2017

/retest

@luxas
Copy link
Member

luxas commented Jul 7, 2017

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kad, luxas

Associated issue: 292

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2017
@luxas
Copy link
Member

luxas commented Jul 7, 2017

/retest

@kad
Copy link
Member Author

kad commented Jul 7, 2017

flakes, flakes... seems like some race conditions in test infra. :(

@ixdy
Copy link
Member

ixdy commented Jul 10, 2017

/retest

1 similar comment
@ixdy
Copy link
Member

ixdy commented Jul 10, 2017

/retest

@ixdy
Copy link
Member

ixdy commented Jul 11, 2017

/retest

1 similar comment
@luxas
Copy link
Member

luxas commented Jul 11, 2017

/retest

@fejta
Copy link
Contributor

fejta commented Jul 11, 2017

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 48196, 42783, 48507, 47719, 46138)

@k8s-github-robot k8s-github-robot merged commit 683b8de into kubernetes:master Jul 12, 2017
@k8s-github-robot
Copy link

@kad PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2017
@luxas luxas mentioned this pull request Jul 24, 2017
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas left review comments

Assignees

@timothysc timothysc

@pipejakob pipejakob

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@kad @k8s-ci-robot @luxas @ixdy @k8s-github-robot @fejta @timothysc @pipejakob @mikedanese