Detect status before enable/disable addon

[fenglixa]

• Fix Usability: check addon status before disable. #4417

• UT Result PASS:
  

• Build & Test PASS:
  

[k8s-ci-robot]

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

[minikube-bot]

Can one of the admins verify this patch?

[RA489]

@minikube-bot ok-to-test

[medyagh]

@minikube-bot ok-to-test

[fenglixa]

@minikube-bot OK to test

[medyagh]

Thanks for contribution, could you pleas add unit testint ? Maybe have a mock enabled addon and try to enable it

[fenglixa]

Thanks for contribution, could you pleas add unit testint ? Maybe have a mock enabled addon and try to enable it

Thanks @medyagh Did you mean following? I think I appended it already. : )

 > ~/Documents/GitHub/minikube >> minikube addons list |grep enable
- addon-manager: enabled
- dashboard: enabled
- default-storageclass: enabled
- storage-provisioner: enabled
 ~/Documents/GitHub/minikube>> minikube addons enable dashboard
💥  addon dashboard was already enabled. 

[medyagh]

Thanks for contribution, could you pleas add unit testint ? Maybe have a mock enabled addon and try to enable it

Thanks @medyagh Did you mean following? I think I appended it already. : )

 > ~/Documents/GitHub/minikube >> minikube addons list |grep enable
- addon-manager: enabled
- dashboard: enabled
- default-storageclass: enabled
- storage-provisioner: enabled
 ~/Documents/GitHub/minikube>> minikube addons enable dashboard
💥  addon dashboard was already enabled. 

I meant like for example for this logic in your code

	if addonStatus && enable {
		console.ErrStyle(console.Conflict, "addon %s was already enabled.", addon.GetAddon())
		os.Exit(exit.Unavailable)
	} else if !addonStatus && !enable {
		console.ErrStyle(console.Conflict, "addon %s was already disabled.", addon.GetAddon())
		os.Exit(exit.Unavailable)
	}

you could put it in a func and then add a TestFunc for it.
that you could do unit testing for that func, here are some example unit tests in minikube. https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/config/util_test.go#L69

you could have a unit test that tries to disable an already disabled addon, and expects it to get err

[medyagh]

Thanks for contribution, could you pleas add unit testint ? Maybe have a mock enabled addon and try to enable it

Thanks @medyagh Did you mean following? I think I appended it already. : )

 > ~/Documents/GitHub/minikube >> minikube addons list |grep enable
- addon-manager: enabled
- dashboard: enabled
- default-storageclass: enabled
- storage-provisioner: enabled
 ~/Documents/GitHub/minikube>> minikube addons enable dashboard
💥  addon dashboard was already enabled. 

I meant like for example for this logic in your code

	if addonStatus && enable {
		console.ErrStyle(console.Conflict, "addon %s was already enabled.", addon.GetAddon())
		os.Exit(exit.Unavailable)
	} else if !addonStatus && !enable {
		console.ErrStyle(console.Conflict, "addon %s was already disabled.", addon.GetAddon())
		os.Exit(exit.Unavailable)
	}

you could put it in a func and then add a TestFunc for it.
that you could do unit testing for that func, here are some example unit tests in minikube. https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/config/util_test.go#L69

you could have a unit test that tries to disable an already disabled addon, and expects it to get err
@fenglixa for example you could have a func called

func isAddonAlreadySet(addon *assets.Addon, enable bool) bool
and then you can move the logic of addonStatus, err := addon.IsEnabled() inside that func

and you would write a unit test for this func in cmd/minikube/cmd/config/util_test.go

TestIsAddonAlreadySet(....)

please let me know if you have any other questions :)

[sharifelgamal]

@minikube-bot OK to test

[fenglixa]

@medyagh Thanks for your comments. I added unit test cases for the new added func. Please help check & review.
@sharifelgamal I addressed the issue in the result of integration test, please help check again. Thanks.
(the reason is get dashboard url will enable dashboard always, which will be failed as it was already enabled)

BR
-Francis

[fenglixa]

09:34:38 | >   - error: failed to create listener: failed to listen on 0.0.0.0:8443: listen tcp 0.0.0.0:8443: bind: address already in use
--- FAIL: TestVersionUpgrade (127.96s)

should be case issue related to #4418

[fenglixa]

@minikube-bot OK to test

[RA489]

/ok-to-test

[medyagh]

Thanks a lot for improvements you made, it looks a lot better just a few more changes.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fenglixa
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 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

[fenglixa]

/retest

[fenglixa]

Test failed is not related to my modification.

/retest

[medyagh]

Thanks for all the changes, please change this to Name(). I think after this change it will be good to go !

[RA489]

@fenglixa can you please squash the commit to one

[fenglixa]

@RA489 squashed done. Thanks

[RA489]

@fenglixa Thanks!

[fenglixa]

Seems it can not take effect when I provide /retest command. Could someone help trigger retest of integration? Thanks

[RA489]

/retest

[fenglixa]

Thanks @RA489. but seems it is also not take effect.

[sharifelgamal]

@minikube-bot OK to test

[fenglixa]

@minikube-bot okay to test

[medyagh]

@minikube-bot OK to test

[medyagh]

@fenglixa Thank you so much for patiently making all the required changes and sorry for the unrelated failing tests. Thank you for the contribution