added condition for the case when user try to enable addons without kubernetes #13508

NikhilSharmaWe · 2022-01-28T08:09:55Z

When user try to enable addons with --no-kubernetes=true an output should be logged informing that it is not possible to enable addons with --no-kubernetes passed. This PR works on logging that output.

k8s-ci-robot · 2022-01-28T08:10:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: NikhilSharmaWe
To complete the pull request process, please assign medyagh after the PR has been reviewed.
You can assign the PR to them by writing /assign @medyagh in a comment when ready.

The full list of commands accepted by this bot can be found 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

minikube-bot · 2022-01-28T08:12:05Z

Can one of the admins verify this patch?

NikhilSharmaWe · 2022-01-28T08:23:18Z

@spowelljr and @SilkeDH is my thinking correct here for adding the improvement.

If yes,

An error Error: cmd/minikube/cmd/start.go:1134:1: cyclomatic complexity 31 of func validateFlags is high (> 30) (gocyclo) is occuring. How can we increase the cyclomatic complexity or what is correct way to deal with it ?

spowelljr · 2022-01-28T18:04:00Z

You have to move the logic you added out to a function to keep the cyclomatic complexity low.

Also, this PR is not catching the case if the user starts minikube and then run minikube addons enable <addon>.

medyagh

thank you for this PR just a lint comment
and plz paste the output the PR before and after this PR

medyagh · 2022-01-28T21:37:27Z

cmd/minikube/cmd/start.go

@@ -1236,6 +1236,10 @@ func validateFlags(cmd *cobra.Command, drvName string) {
 		exit.Message(reason.Usage, "Sorry, please set the --output flag to one of the following valid options: [text,json]")
 	}

+	if viper.GetStringSlice(config.AddonListFlag) != nil && viper.GetBool(noKubernetes) {


consider adding it to a helper func to fix the lint error "cyclomatic complexity 31"

helper could be caled
validateNoAddonsForNoKubernetes(config...)

@medyagh

Is the function validateNoAddonsForNoKubernetes() correctly added ?

I have called func validateNoAddonsForNoKubernetes() inside the func provisionWithDriver(), func validateFlags() was also called here, so is it a correct place to call it ?

NikhilSharmaWe · 2022-01-29T05:01:49Z

@spowelljr I am not able to understand your point when you say :

Also, this PR is not catching the case if the user starts minikube and then run minikube addons enable <addon>.

Could you explain me that what case actually we have tackled in this PR till now and how can we deal with the other case you have mentioned in this comment ?

spowelljr · 2022-01-31T17:44:19Z

@spowelljr I am not able to understand your point when you say :

Also, this PR is not catching the case if the user starts minikube and then run minikube addons enable <addon>.

Could you explain me that what case actually we have tackled in this PR till now and how can we deal with the other case you have mentioned in this comment ?

Right now you're checking the case where the user runs:

minikube start --addons metallb

But also need to check for the case where user starts minikube and then enables the addon after:

minikube start
minikube addons enable metallb

NikhilSharmaWe · 2022-02-01T04:53:48Z

@spowelljr

You suggested in this comment of yours that use function config.Load() to find out that --no-kubernetes has been set or not. Function config.Load() return value of type ClusterConfig and in this type I am not able to find out how to figure out whether --no-kubernetes has been set or not.
I have done something different here, could you please review the method I have followed.

spowelljr · 2022-02-01T19:01:07Z

cmd/minikube/cmd/config/enable.go

@@ -40,6 +40,9 @@ var addonsEnableCmd = &cobra.Command{
 		if len(args) != 1 {
 			exit.Message(reason.Usage, "usage: minikube addons enable ADDON_NAME")
 		}
+		if ClusterFlagValue() == constants.NoKubernetesVersion {


ClusterFlagValue() returns the profile, so it doesn't make any sense to compare that against a Kubernetes version.

spowelljr · 2022-02-01T19:02:08Z

cmd/minikube/cmd/start.go

@@ -1652,3 +1653,9 @@ func exitGuestProvision(err error) {
 	}
 	exit.Error(reason.GuestProvision, "error provisioning guest", err)
 }
+
+func validateNoAddonsForNoKubernetes() {
+	if viper.GetStringSlice(config.AddonListFlag) != nil && viper.GetBool(noKubernetes) {


viper.GetStringSlice(config.AddonListFlag) isn't nil when no addons flag is passed, try running minikube start --no-kubernetes yourself.

spowelljr · 2022-02-01T19:11:13Z

@spowelljr

You suggested in this comment of yours that use function config.Load() to find out that --no-kubernetes has been set or not. Function config.Load() return value of type ClusterConfig and in this type I am not able to find out how to figure out whether --no-kubernetes has been set or not.

I have done something different here, could you please review the method I have followed.

ClusterConfig.KubernetesConfig.KubernetesVersion is set to constants.NoKubernetesVersion to indicate that --no-kubernetes is set.

…ubernetes

NikhilSharmaWe · 2022-02-05T05:22:16Z

@spowelljr and @medyagh

Made changes in the logic in enable.go.
Also added the same logic inside the function validateNoAddonsForNoKubernetes in start.go.

Do these changes solves the issue occurring before ? Please inform.

k8s-triage-robot · 2022-05-08T05:33:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-06-07T06:07:44Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

spowelljr · 2022-06-08T18:21:18Z

cmd/minikube/cmd/config/enable.go

@@ -40,6 +40,11 @@ var addonsEnableCmd = &cobra.Command{
 		if len(args) != 1 {
 			exit.Message(reason.Usage, "usage: minikube addons enable ADDON_NAME")
 		}
+		if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {


Suggested change

if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {

if clusterConfig, err := config.Load(ClusterFlagValue()); err == nil {

As per Go naming conventions

spowelljr · 2022-06-08T18:21:26Z

cmd/minikube/cmd/config/enable.go

@@ -40,6 +40,11 @@ var addonsEnableCmd = &cobra.Command{
 		if len(args) != 1 {
 			exit.Message(reason.Usage, "usage: minikube addons enable ADDON_NAME")
 		}
+		if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {
+			if ClusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {


Suggested change

if ClusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {

if clusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {

spowelljr · 2022-06-08T18:21:38Z

cmd/minikube/cmd/start.go

@@ -1652,3 +1653,11 @@ func exitGuestProvision(err error) {
 	}
 	exit.Error(reason.GuestProvision, "error provisioning guest", err)
 }
+
+func validateNoAddonsForNoKubernetes() {
+	if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {


Suggested change

if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {

if clusterConfig, err := config.Load(ClusterFlagValue()); err == nil {

spowelljr · 2022-06-08T18:21:45Z

cmd/minikube/cmd/start.go

+
+func validateNoAddonsForNoKubernetes() {
+	if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {
+		if ClusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {


Suggested change

if ClusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {

if clusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {

k8s-triage-robot · 2022-07-08T18:34:00Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-07-08T18:34:18Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 28, 2022

k8s-ci-robot requested review from klaases and prasadkatti January 28, 2022 08:10

medyagh requested changes Jan 28, 2022

View reviewed changes

NikhilSharmaWe force-pushed the addons branch 3 times, most recently from c980f7e to f7526ff Compare January 29, 2022 04:51

NikhilSharmaWe force-pushed the addons branch from f7526ff to f8641e6 Compare February 1, 2022 04:52

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 1, 2022

NikhilSharmaWe requested a review from medyagh February 1, 2022 05:25

spowelljr requested changes Feb 1, 2022

View reviewed changes

NikhilSharmaWe force-pushed the addons branch 3 times, most recently from 8b15e2c to a3a8e09 Compare February 5, 2022 04:28

added condition for the case when user try to enable addons without k…

9931451

…ubernetes

NikhilSharmaWe force-pushed the addons branch from a3a8e09 to 9931451 Compare February 5, 2022 05:09

NikhilSharmaWe requested a review from spowelljr February 5, 2022 09:02

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 8, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 7, 2022

spowelljr requested changes Jun 8, 2022

View reviewed changes

k8s-ci-robot closed this Jul 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added condition for the case when user try to enable addons without kubernetes #13508

added condition for the case when user try to enable addons without kubernetes #13508

NikhilSharmaWe commented Jan 28, 2022 •

edited

k8s-ci-robot commented Jan 28, 2022

minikube-bot commented Jan 28, 2022

NikhilSharmaWe commented Jan 28, 2022 •

edited

spowelljr commented Jan 28, 2022

medyagh left a comment

medyagh Jan 28, 2022

NikhilSharmaWe Jan 29, 2022

NikhilSharmaWe commented Jan 29, 2022 •

edited

spowelljr commented Jan 31, 2022

NikhilSharmaWe commented Feb 1, 2022

spowelljr Feb 1, 2022

spowelljr Feb 1, 2022

spowelljr commented Feb 1, 2022

NikhilSharmaWe commented Feb 5, 2022 •

edited

k8s-triage-robot commented May 8, 2022

k8s-triage-robot commented Jun 7, 2022

spowelljr Jun 8, 2022

spowelljr Jun 8, 2022

spowelljr Jun 8, 2022

spowelljr Jun 8, 2022

k8s-triage-robot commented Jul 8, 2022

k8s-ci-robot commented Jul 8, 2022

	if ClusterConfig, err := config.Load(ClusterFlagValue()); err == nil {
	if clusterConfig, err := config.Load(ClusterFlagValue()); err == nil {

	if ClusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {
	if clusterConfig.KubernetesConfig.KubernetesVersion == constants.NoKubernetesVersion {

added condition for the case when user try to enable addons without kubernetes #13508

added condition for the case when user try to enable addons without kubernetes #13508

Conversation

NikhilSharmaWe commented Jan 28, 2022 • edited

k8s-ci-robot commented Jan 28, 2022

minikube-bot commented Jan 28, 2022

NikhilSharmaWe commented Jan 28, 2022 • edited

spowelljr commented Jan 28, 2022

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

NikhilSharmaWe commented Jan 29, 2022 • edited

spowelljr commented Jan 31, 2022

NikhilSharmaWe commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spowelljr commented Feb 1, 2022

NikhilSharmaWe commented Feb 5, 2022 • edited

k8s-triage-robot commented May 8, 2022

k8s-triage-robot commented Jun 7, 2022

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

k8s-triage-robot commented Jul 8, 2022

k8s-ci-robot commented Jul 8, 2022

NikhilSharmaWe commented Jan 28, 2022 •

edited

NikhilSharmaWe commented Jan 28, 2022 •

edited

NikhilSharmaWe commented Jan 29, 2022 •

edited

NikhilSharmaWe commented Feb 5, 2022 •

edited