Exit if --kubernetes-version is older than the oldest supported version #4759
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
cncf-cla: yes
|
Hi @serhatcetinkaya. 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 Once the patch is verified, the new status will be reflected by the 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: serhatcetinkaya The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
|
Can one of the admins verify this patch? |
tstromberg
suggested changes
Jul 15, 2019
cmd/minikube/cmd/start.go
Outdated
| exit.WithCode(exit.Data, "Unable to parse oldest Kubernetes version from constants: %v", err) | ||
| } | ||
|
|
||
| superOldVersionLimit := semver.Version{Major: 1, Minor: 9, Patch: 0} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
Thanks for the clarification @medyagh @tstromberg, I am working on this, I will submit as soon as I finish.
There was a problem hiding this comment.
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.
|
@minikube-bot OK to test |
medyagh
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
Travis tests have failedHey @serhatcetinkaya, 1st Buildmake testTravisBuddy Request Identifier: 599bb650-ccd8-11e9-a9d8-278aeb0f3435 |
|
Hi, sorry for the long delay. I added an if check to control I guess this is because of a vote to use the Github repos as primary repo and this one is moved to Github. Do you have any solutions for this ? |
Travis tests have failedHey @serhatcetinkaya, 1st Buildmake testTravisBuddy Request Identifier: 3d6ba970-ccdf-11e9-a9d8-278aeb0f3435 |
Travis tests have failedHey @serhatcetinkaya, 1st Buildmake testTravisBuddy Request Identifier: 8d45a390-cce1-11e9-a9d8-278aeb0f3435 |
Travis tests have failedHey @serhatcetinkaya, 1st Buildmake testTravisBuddy Request Identifier: 0c9c0ee0-cce2-11e9-a9d8-278aeb0f3435 |
Travis tests have failedHey @serhatcetinkaya, 1st Buildmake testTravisBuddy Request Identifier: 919ae200-cce3-11e9-a9d8-278aeb0f3435 |
tstromberg
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Looks great. Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is intended as a fix for the issue described in #4673.
If the version specified by the user is less than
constants.OldestKubernetesVersionwe print a warning to the user but continue execution, if the specified version is less thanv1.9.0we exit with a proper message.Can you please review @medyagh :)