--kubernetes-version: assume latest patch version if not specified by user #16569

JudahNour · 2023-05-23T19:35:01Z

Before:

$ mk start --kubernetes-version=1.26
😄  minikube v1.30.1 on Debian rodete (amd64)

❌  Exiting due to MK_USAGE: Unable to parse "1.26": No Major.Minor.Patch elements found

After:

$ mk start --kubernetes-version=1.26
😄  minikube v1.30.1 on Debian rodete (amd64)
👉  Using Kubernetes 1.26.5 since patch version was unspecified
✨  Automatically selected the docker driver
📌  Using Docker driver with root privileges
👍  Starting control plane node minikube in cluster minikube
🚜  Pulling base image ...
🔥  Creating docker container (CPUs=2, Memory=8000MB) ...
🐳  Preparing Kubernetes v1.26.5 on Docker 24.0.0 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🔎  Verifying Kubernetes components...
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" cluster and "default" namespace by default

Before:

$ mk start --kubernetes-version=1.50
😄  minikube v1.30.1 on Debian rodete (amd64)

❌  Exiting due to MK_USAGE: Unable to parse "1.50": No Major.Minor.Patch elements found

After:

$ mk start --kubernetes-version=1.50
😄  minikube v1.30.1 on Debian rodete (amd64)

❌  Exiting due to MK_PATCH_NOT_FOUND: Unable to detect the latest patch release for specified major.minor version v1.50
💡  Suggestion: Specify --kubernetes-version in v<major>.<minor.<build> form. example: 'v1.1.14'

After:

$ mk start --kubernetes-version=hahahahha
😄  minikube v1.30.1 on Debian rodete (amd64)

❌  Exiting due to MK_USAGE: Unable to parse "hahahahha": No Major.Minor.Patch elements found

linux-foundation-easycla · 2023-05-23T19:35:04Z

The committers listed above are authorized under a signed CLA.

✅ login: JudahNour (a81e4f7)

k8s-ci-robot · 2023-05-23T19:35:09Z

Welcome @JudahNour!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-05-23T19:35:10Z

Hi @JudahNour. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

medyagh

lets make

medyagh · 2023-05-23T19:37:31Z

cmd/minikube/cmd/start.go

@@ -1757,8 +1757,17 @@ $ minikube config unset kubernetes-version`)
 	} else if strings.EqualFold(strings.ToLower(paramVersion), "latest") || strings.EqualFold(strings.ToLower(paramVersion), "newest") {
 		paramVersion = constants.NewestKubernetesVersion
 	}
-
-	nvs, err := semver.Make(strings.TrimPrefix(paramVersion, version.VersionPrefix))
+	kubernetesSemver := strings.TrimPrefix(paramVersion, version.VersionPrefix)


lets make sure we notify the user that we chose latest patch for them since they didn't provide a patch version

also a better PR title would be, "--kubernetes-version: assume latest patch ver if not specified by user"

Done. I abstracted out the majorMinor check and print it out in validateKubernetesVersion. The function uses the param version so this could be slightly redundant as we do that processing of the param in getKubernetesVersion already and thus redo some of it in validateKubernetesVersion so we can run the check there as well.

spowelljr · 2023-05-23T19:37:54Z

/check-cla

minikube-bot · 2023-05-23T19:39:05Z

Can one of the admins verify this patch?

…ubernetesVersions

cmd/minikube/cmd/start.go

medyagh · 2023-05-24T18:22:07Z

cmd/minikube/cmd/start.go

@@ -1659,6 +1659,13 @@ func validateKubernetesVersion(old *config.ClusterConfig) {
 	newestVersion := semver.MustParse(strings.TrimPrefix(constants.NewestKubernetesVersion, version.VersionPrefix))
 	zeroVersion := semver.MustParse(strings.TrimPrefix(constants.NoKubernetesVersion, version.VersionPrefix))

+	paramVersion := viper.GetString(kubernetesVersion)


while we are at there could u plz check if captical V works too ? if not I suggest while we are touching this, convert the version to lowercase

Capital V does not work. Do we want it so that the paramVersion is completely set to lowercase so that this is accounted for? Or do we want it to specifically set the first character to lowercase? I think the former would mean someone could input something like "StAble" as the flag and it would recognize it as "stable"

yeah lets convert the user input to lowercase for all cases, that makes our life easier

for the major.minor given

JudahNour · 2023-05-30T16:32:38Z

cmd/minikube/cmd/start.go

+			kubernetesSemver = potentialPatch
+		} else {
+			return potentialPatch, ErrPatchNotFound
+		}
 	}
 	nvs, err := semver.Make(kubernetesSemver)
 	if err != nil {


Not sure if here we want to send back an error here instead of just exiting. With the error return value it could be better to now return an error and handle it outside getKubernetesVersion

good point and good catch ! I think since it is already been implemented like that we can leave this one alone, and if we want we can refactor it in a follow up PR (for sake of keeping this PR small)

cmd/minikube/cmd/start.go

medyagh · 2023-05-30T17:23:37Z

cmd/minikube/cmd/start.go

+			kubernetesSemver = potentialPatch
+		} else {
+			return potentialPatch, ErrPatchNotFound
+		}
 	}
 	nvs, err := semver.Make(kubernetesSemver)
 	if err != nil {


good point and good catch ! I think since it is already been implemented like that we can leave this one alone, and if we want we can refactor it in a follow up PR (for sake of keeping this PR small)

cmd/minikube/cmd/start.go

medyagh · 2023-05-30T17:28:09Z

@JudahNour thank for updating the PR, do you mind also re-doing the Before/After after comming the last changes to ensure it is still good

cmd/minikube/cmd/start.go

cmd/minikube/cmd/start_flags.go

cmd/minikube/cmd/start_test.go

Corrected bug with version prefixes

spowelljr

Run make generate-docs to update translations

cmd/minikube/cmd/start.go

cmd/minikube/cmd/start_test.go

cmd/minikube/cmd/start.go

cmd/minikube/cmd/start_test.go

medyagh · 2023-05-31T18:18:17Z

/ok-to-test

minikube-pr-bot · 2023-05-31T19:18:39Z

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16569) |
+----------------+----------+---------------------+
| minikube start | 53.5s    | 52.7s               |
| enable ingress | 27.7s    | 28.1s               |
+----------------+----------+---------------------+

Times for minikube start: 53.3s 53.4s 56.2s 52.2s 52.2s
Times for minikube (PR 16569) start: 54.2s 52.6s 51.3s 52.2s 53.2s

Times for minikube ingress: 28.2s 27.2s 27.2s 28.2s 27.7s
Times for minikube (PR 16569) ingress: 28.2s 28.2s 28.3s 27.7s 28.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16569) |
+----------------+----------+---------------------+
| minikube start | 24.1s    | 23.0s               |
| enable ingress | 20.8s    | 21.2s               |
+----------------+----------+---------------------+

Times for minikube start: 21.8s 25.3s 22.2s 25.3s 25.8s
Times for minikube (PR 16569) start: 21.6s 22.1s 21.8s 24.5s 24.7s

Times for minikube ingress: 20.9s 20.4s 20.9s 21.9s 19.9s
Times for minikube (PR 16569) ingress: 21.4s 20.9s 21.9s 20.9s 20.9s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16569) |
+----------------+----------+---------------------+
| minikube start | 22.8s    | 22.1s               |
| enable ingress | 32.4s    | 31.4s               |
+----------------+----------+---------------------+

Times for minikube start: 20.4s 24.8s 21.3s 23.6s 24.0s
Times for minikube (PR 16569) start: 23.6s 23.0s 20.1s 22.7s 21.4s

Times for minikube ingress: 31.4s 46.4s 31.4s 19.4s 33.4s
Times for minikube (PR 16569) ingress: 31.4s 31.4s 31.3s 31.4s 31.4s

minikube-pr-bot · 2023-05-31T20:23:13Z

These are the flake rates of all failed tests.

Environment	Failed Tests	Flake Rate (%)
Docker_Linux_crio	TestFunctional/parallel/ImageCommands/ImageBuild (gopogh)	0.62 (chart)
Docker_Linux_crio_arm64	TestPause/serial/SecondStartNoReconfiguration (gopogh)	22.88 (chart)

To see the flake rates of all tests by environment, click here.

k8s-ci-robot · 2023-06-01T17:49:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JudahNour, medyagh

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~~ [medyagh]

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

medyagh · 2023-06-01T17:50:18Z

looks good to me ! Congratulations on your first contribution ! looking forward to see more !

accept kubernetes version without patch version

a81e4f7

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 23, 2023

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 May 23, 2023

k8s-ci-robot requested review from afbjorklund and medyagh May 23, 2023 19:35

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 23, 2023

medyagh requested changes May 23, 2023

View reviewed changes

JudahNour changed the title ~~accept kubernetes version without patch version~~ --kubernetes-version: assume latest patch version if not specified by user May 23, 2023

Modified so alerts user that the patch version is assumed

13c0d58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 24, 2023

JudahNour added 2 commits May 24, 2023 16:44

modified so alerts user that the patch version is assumed

3afc728

Merge remote-tracking branch 'origin/addKubernetesVersions' into addK…

0500766

…ubernetesVersions

medyagh requested changes May 24, 2023

View reviewed changes

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

medyagh requested changes May 24, 2023

View reviewed changes

JudahNour added 2 commits May 24, 2023 20:26

--kubernetes-version: making case insensitive and refactoring handling

cbd5778

Modified to alert user and exit when there is no associated patch

e01610a

for the major.minor given

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 25, 2023

JudahNour commented May 30, 2023

View reviewed changes

medyagh requested changes May 30, 2023

View reviewed changes

spowelljr requested changes May 30, 2023

View reviewed changes

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

spowelljr requested changes May 30, 2023

View reviewed changes

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

spowelljr requested changes May 30, 2023

View reviewed changes

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

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

Add testing for isTwoDigitSemver and changed messages to user.

c2c3f29

Corrected bug with version prefixes

spowelljr requested changes May 30, 2023

View reviewed changes

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

Refactored code, updated error messages, and generated docs

c81a317

spowelljr requested changes May 31, 2023

View reviewed changes

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

cmd/minikube/cmd/start_test.go Show resolved Hide resolved

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

Refactored code and modified/added tests

852d919

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 31, 2023

medyagh approved these changes Jun 1, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2023

medyagh merged commit 0168d63 into kubernetes:master Jun 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--kubernetes-version: assume latest patch version if not specified by user #16569

--kubernetes-version: assume latest patch version if not specified by user #16569

JudahNour commented May 23, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 23, 2023 •

edited

Loading

k8s-ci-robot commented May 23, 2023

k8s-ci-robot commented May 23, 2023

medyagh left a comment

medyagh May 23, 2023

medyagh May 23, 2023

JudahNour May 24, 2023

spowelljr commented May 23, 2023

minikube-bot commented May 23, 2023

medyagh May 24, 2023

JudahNour May 24, 2023

medyagh May 25, 2023

JudahNour May 30, 2023

medyagh May 30, 2023

medyagh May 30, 2023

medyagh commented May 30, 2023

spowelljr left a comment

medyagh commented May 31, 2023

minikube-pr-bot commented May 31, 2023

minikube-pr-bot commented May 31, 2023

k8s-ci-robot commented Jun 1, 2023

medyagh commented Jun 1, 2023

--kubernetes-version: assume latest patch version if not specified by user #16569

--kubernetes-version: assume latest patch version if not specified by user #16569

Conversation

JudahNour commented May 23, 2023 • edited Loading

Before:

After:

Before:

After:

After:

linux-foundation-easycla bot commented May 23, 2023 • edited Loading

k8s-ci-robot commented May 23, 2023

k8s-ci-robot commented May 23, 2023

medyagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spowelljr commented May 23, 2023

minikube-bot commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medyagh commented May 30, 2023

spowelljr left a comment

Choose a reason for hiding this comment

medyagh commented May 31, 2023

minikube-pr-bot commented May 31, 2023

minikube-pr-bot commented May 31, 2023

k8s-ci-robot commented Jun 1, 2023

medyagh commented Jun 1, 2023

JudahNour commented May 23, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 23, 2023 •

edited

Loading