Improve error handling for versions without `v` prefix #3766

dimitropoulos · 2019-02-27T20:16:54Z

I always forget whether the v is required syntax. When you start minikube without the v, it currently looks like this:

$ minikube start --kubernetes-version 1.10.0
Starting local Kubernetes 1.10.0 cluster...
Starting VM...
Getting VM IP address...
Moving files into cluster...
E0227 14:07:52.609622  117027 start.go:302] Error updating cluster:  generating kubeadm cfg: parsing kubernetes version: parsing kubernetes version: strconv.ParseUint: parsing "": invalid syntax


minikube failed :( exiting with error code 1

I added a test or two and tried to make the error message clearer, since, after all, it isn't "" that I passed so I (as the user) don't have much to go on to understand what it's talking about in the current error.

That said.. I would be happy to just fix things up to allow either the v or the no-v versions accepted for this flag. I only didn't do so because I was unsure if this was some kind of design choice. In the two places in the code (both edited in this PR) where the version is used, it is immediately stripped of its v anyway.

minikube-bot · 2019-02-27T20:17:04Z

Can one of the admins verify this patch?

k8s-ci-robot · 2019-02-27T20:17:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dimitropoulos
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jimmidyson

If they are not already assigned, you can assign the PR to them by writing /assign @jimmidyson in a comment when ready.

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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cmd/minikube/cmd/start.go

tstromberg · 2019-02-28T00:02:05Z

pkg/minikube/bootstrapper/kubeadm/versions.go

@@ -25,6 +25,7 @@ import (

 	"github.com/blang/semver"
 	"github.com/golang/glog"
+	"github.com/kubernetes/minikube/pkg/version"


Is this PR missing a file?

I'm not sure. could you please rephrase? I am hoping this file (and the other kubeadm one) isn't from somewhere else.

The travis test failed with:

Running go tests... pkg/minikube/bootstrapper/kubeadm/versions.go:28:2: cannot find package "github.com/kubernetes/minikube/pkg/version" in any of: /home/travis/gopath/src/k8s.io/minikube/vendor/github.com/kubernetes/minikube/pkg/version (vendor tree) /home/travis/.gimme/versions/go1.12.linux.amd64/src/github.com/kubernetes/minikube/pkg/version (from $GOROOT) /home/travis/gopath/src/github.com/kubernetes/minikube/pkg/version (from $GOPATH) make: *** [test] Error 1 The command "make test" exited with 2.

I'm not sure why this is happening though, because that package does exist as far as I know. Hmm...

I mean.. it's not totally necessary because I feel pretty confident the version prefix isn't changing from v any time soon. I just used it because I noticed it was there and figured it could (if nothing else) be clearer instead of looking like some random magic-character, you know?

To pass the tests, this should be:

"k8s.io/minikube/pkg/version"

Thanks!

tstromberg · 2019-03-01T21:22:21Z

@minikube-bot OK to test

tstromberg · 2019-04-11T18:30:18Z

Marking as WIP until @dimitropoulos has a chance to rebase.

dimitropoulos · 2019-04-11T18:46:09Z

ok so I rebased it but there seem to be some other changes that attempt to catch the same thing that have been done since I started... I'll leave it to you to decide how you want to proceed.

tstromberg · 2019-04-30T19:43:34Z

A coworker clued me into what is happening with this error message:

pkg/minikube/bootstrapper/kubeadm/versions.go:28:2: cannot find package "github.com/kubernetes/minikube/pkg/version" in any of: /home/travis/gopath/src/k8s.io/minikube/vendor/github.com/kubernetes/minikube/pkg/version (vendor tree) /home/travis/.gimme/versions/go1.12.3.linux.amd64/src/github.com/kubernetes/minikube/pkg/version (from $GOROOT) /home/travis/gopath/src/github.com/kubernetes/minikube/pkg/version (from $GOPATH)

The import is using github.com/kubernetes instead of k8s.io/minikube. It is admittedly a bit weird to have this alias, but it should fix the broken tests. Do you mind changing it?

You'll want to move which directory minikube source is stored in:

https://github.com/kubernetes/minikube/blob/master/docs/contributors/build_guide.md#building-from-source

I suspect when we move to Go modules that this will go away.

dimitropoulos · 2019-04-30T23:14:17Z

perhaps I misunderstood your directions (I don't, myself, understand what the actual problem is either) but I tried to update it and it seems to still fail.

tstromberg · 2019-04-30T23:18:28Z

Excellent, it looks like the failure is at least different now:

pkg/minikube/bootstrapper/kubeadm/versions.go:164:14: invalid operation: input[0] != "k8s.io/minikube/pkg/version".VersionPrefix (mismatched types byte and string)
FAIL	k8s.io/minikube/cmd/minikube/cmd [build failed]

I think this can be fixed by using this for your comparison:

if strings.HasPrefix(input, version.VersionPrefix) {

If you run make test locally, do you see the same error?

tstromberg · 2019-05-22T18:31:22Z

Is this PR still being worked on? I believe this may be a 1-line change away from merging.

dimitropoulos · 2019-05-29T18:36:27Z

I made the change you suggested and it appears to still be failing. when I test it locally I get:

cannot convert "k8s.io/minikube/pkg/version".VersionPrefix (type untyped string) to type byte

but I can't seem to figure a way to fix it.

tstromberg · 2019-05-29T23:08:44Z

cmd/minikube/cmd/start.go

@@ -427,6 +427,11 @@ func validateKubernetesVersions(old *cfg.Config) string {
 	if nv == "" {
 		nv = constants.DefaultKubernetesVersion
 	}
+
+	if nv[0] != version.VersionPrefix {


To avoid the bytes/string error, this should match the check in versions.go:

if !strings.HasPrefix(nv, version.VersionPrefix) {

Alternatively, why not just allow a version to be specified without a leading v? We know what the user wants, so instead of raising the error, we could just give it to them and make their life easier:

if !strings.HasPrefix(nv, version.VersionPrefix) { nv = version.VersionPrefix + nv }

tstromberg · 2019-05-29T23:15:50Z

pkg/minikube/bootstrapper/kubeadm/versions.go

-	v, err := semver.Make(version[1:])
+// ParseKubernetesVersion validates the kubernetes version as legitimate
+func ParseKubernetesVersion(input string) (semver.Version, error) {
+	if !strings.HasPrefix(input, version.VersionPrefix) {


I think this check can be removed: it isn't necessary to check for the leading 'v' here because it is stripped away immediately afterwards. Just continue to use strings.TrimPrefix, and it will handle both cases perfectly.

tstromberg · 2019-06-24T02:34:31Z

As I haven't heard anything recently and I bumped into this issue myself, I took inspiration from this PR to create a new PR: #4568

dimitropoulos · 2019-06-24T16:36:18Z

great. thanks @tstromberg

k8s-ci-robot requested review from balopat and tstromberg February 27, 2019 20:17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2019

tstromberg reviewed Feb 28, 2019

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

tstromberg reviewed Feb 28, 2019

View reviewed changes

dimitropoulos force-pushed the versionPrefixHandling branch from a2e966b to e11f258 Compare February 28, 2019 14:52

tstromberg added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2019

tstromberg changed the title ~~improves error handling for versions without v prefix~~ WIP: improves error handling for versions without v prefix Apr 11, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2019

improves error handling for versions without v prefix

147f686

dimitropoulos force-pushed the versionPrefixHandling branch from e11f258 to 147f686 Compare April 11, 2019 18:45

updates import path

5fe9eae

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2019

tstromberg mentioned this pull request May 22, 2019

Parsing of Kubernetes version strings is brittle and doesn't display useful error messages #3253

Closed

uses strings.HasPrefix rather than byte comparison

8375551

tstromberg suggested changes May 29, 2019

View reviewed changes

tstromberg changed the title ~~WIP: improves error handling for versions without v prefix~~ Improves error handling for versions without v prefix May 29, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2019

tstromberg changed the title ~~Improves error handling for versions without v prefix~~ Improve error handling for versions without v prefix May 29, 2019

tstromberg mentioned this pull request Jun 24, 2019

Allow --kubernetes-version to be specified without the leading v #4568

Merged

tstromberg closed this Jun 24, 2019

dimitropoulos deleted the versionPrefixHandling branch June 24, 2019 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling for versions without `v` prefix #3766

Improve error handling for versions without `v` prefix #3766

dimitropoulos commented Feb 27, 2019

minikube-bot commented Feb 27, 2019

k8s-ci-robot commented Feb 27, 2019

tstromberg Feb 28, 2019

dimitropoulos Feb 28, 2019

tstromberg Mar 1, 2019

dimitropoulos Mar 1, 2019

tstromberg Apr 25, 2019

tstromberg commented Mar 1, 2019

tstromberg commented Apr 11, 2019

dimitropoulos commented Apr 11, 2019

tstromberg commented Apr 30, 2019

dimitropoulos commented Apr 30, 2019

tstromberg commented Apr 30, 2019

tstromberg commented May 22, 2019

dimitropoulos commented May 29, 2019

tstromberg May 29, 2019

tstromberg May 29, 2019

tstromberg commented Jun 24, 2019

dimitropoulos commented Jun 24, 2019

Improve error handling for versions without v prefix #3766

Improve error handling for versions without v prefix #3766

Conversation

dimitropoulos commented Feb 27, 2019

minikube-bot commented Feb 27, 2019

k8s-ci-robot commented Feb 27, 2019

tstromberg Feb 28, 2019

Choose a reason for hiding this comment

dimitropoulos Feb 28, 2019

Choose a reason for hiding this comment

tstromberg Mar 1, 2019

Choose a reason for hiding this comment

dimitropoulos Mar 1, 2019

Choose a reason for hiding this comment

tstromberg Apr 25, 2019

Choose a reason for hiding this comment

tstromberg commented Mar 1, 2019

tstromberg commented Apr 11, 2019

dimitropoulos commented Apr 11, 2019

tstromberg commented Apr 30, 2019

dimitropoulos commented Apr 30, 2019

tstromberg commented Apr 30, 2019

tstromberg commented May 22, 2019

dimitropoulos commented May 29, 2019

tstromberg May 29, 2019

Choose a reason for hiding this comment

tstromberg May 29, 2019

Choose a reason for hiding this comment

tstromberg commented Jun 24, 2019

dimitropoulos commented Jun 24, 2019

Improve error handling for versions without `v` prefix #3766

Improve error handling for versions without `v` prefix #3766