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

Exit if --kubernetes-version is older than the oldest supported version #4759

Merged
merged 6 commits into from
Sep 5, 2019
Merged

Exit if --kubernetes-version is older than the oldest supported version #4759

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
deead30
start: compare specified kubernetes version with oldest supported ver…
serhatcetinkaya Jul 14, 2019
578e325
Merge remote-tracking branch 'upstream/master'
serhatcetinkaya Sep 1, 2019
2230a52
start: check force flag while comparing specified version
serhatcetinkaya Sep 1, 2019
0b829cc
fix warnings
serhatcetinkaya Sep 1, 2019
14b1d84
start.go rename exit.WithCode with exit.WithCodeT
serhatcetinkaya Sep 1, 2019
23cc018
start.go make err type usable for exit.withCodeT
serhatcetinkaya Sep 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions cmd/minikube/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,20 @@ func validateKubernetesVersions(old *cfg.Config) (string, bool) {
return nv, isUpgrade
}

oldestVersion, err := semver.Make(strings.TrimPrefix(constants.OldestKubernetesVersion, version.VersionPrefix))
if err != nil {
exit.WithCode(exit.Data, "Unable to parse oldest Kubernetes version from constants: %v", err)
}

superOldVersionLimit := semver.Version{Major: 1, Minor: 9, Patch: 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this PR!

If possible, I would rather not have two effective "oldest" constants. The intent of constants.OldestKubernetesVersion was to have a constant that defined the oldest possible version. If v1.9.x works, the variable should reflect that state.

Do you mind updating constants.OldestKubernetesVersion to v1.9.0 so that the intent is met and the logic simple? If it happens to fail tests, we can stick with v1.10.x and add a --force flag so that users can still bypass the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Thomas, thanks for the review.

As far as I understand this is the behavior requested in the original issue #4673. According to your input we can remove the check for superOldVersionLimit and just return a warning (without exit) to the user if the specified version is less than constants.OldestKubernetesVersion.

Can you check this comment to make things clear about what needs to be done #4673 (comment)

Copy link
Member

@medyagh medyagh Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @serhatcetinkaya sorry for the confusion, I think I contributed to the confusion ! my fault !

Thomas is right, rethinking this I believe it would be hard to maintain two separate versions of supported kuberentes.

the original reason we created this issue was someone was trying to start an oddly old kubernetes by mistake (1.2 instead of 1.12)

here is my current new suggestion based on @tstromberg input:

we need to exit on oldest supported kubernets version however we need to infrom the user in the exit message, that to override this check please add "--force" so they can still run whatever version they want ( which we don't support at their own risk )

Copy link
Contributor

Choose a reason for hiding this comment

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

@serhatcetinkaya - Yes, that sounds like the right way to go. I spoke to Medya about this yesterday and he seemed willing to accept it as well. I've added my thoughts to the bug. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification @medyagh @tstromberg, I am working on this, I will submit as soon as I finish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huzzah! start.go now has support for a --force flag. If you change the logic to use it - I think we should merge this ASAP.

if viper.GetBool(force) {
  out.WarningT("Kubernetes {{.version}} is not supported by this release of minikube", ...)
} else {
  exit.WithCode(exit.Data, "Sorry, Kubernetes {{.version}} is not supported by this release of minikube")
}

Please let me know if you need any help.

if nvs.LT(superOldVersionLimit) {
exit.WithCode(exit.Data, "Specified Kubernetes version is not supported in minikube: %q", rawVersion)
}

if nvs.LT(oldestVersion) {
console.ErrT(console.Conflict, "Specified Kubernetes version {{.specified}} is less than the oldest supported version: {{.oldest}}", console.Arg{"specified": nvs, "oldest": constants.OldestKubernetesVersion})
}

ovs, err := semver.Make(strings.TrimPrefix(old.KubernetesConfig.KubernetesVersion, version.VersionPrefix))
if err != nil {
glog.Errorf("Error parsing old version %q: %v", old.KubernetesConfig.KubernetesVersion, err)
Expand Down