start: Comma-delimited --addons silently ignored

[tstromberg]

Oddly, our --addons flag does not allow comma-delimited addons. Worse, it completely ignores your choice rather than returning an error:

 ./out/minikube start --addons=dashboard,efk,freshpod,istio,istio-provisioner,olm --memory=16384 --driver=hyperkit                    5.3s  Thu Sep 24 12:12:02 2020
😄  minikube v1.13.1 on Darwin 10.15.6
✨  Using the hyperkit driver based on user configuration
👍  Starting control plane node minikube in cluster minikube
🔥  Creating hyperkit VM (CPUs=2, Memory=16384MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.19.2 on Docker 19.03.12 ...
🔎  Verifying Kubernetes components...
🌟  Enabled addons: default-storageclass, storage-provisioner

❗  /usr/local/bin/kubectl is version 1.16.6-beta.0, which may have incompatibilites with Kubernetes 1.19.2.
💡  Want kubectl v1.19.2? Try 'minikube kubectl -- get pods -A'
🏄  Done! kubectl is now configured to use "minikube" by default

[tstromberg]

It may be possible to fix this by switching this line in cmd/minikube/cmd/start_flags.go to use a Slice instead of an Array, but I suspect there are devils in the implementation details:

startCmd.Flags().StringArrayVar(&config.AddonList, "addons", nil, "Enable addons. see minikube addons list for a list of valid addon names.")

Someone will also need to make sure that the existing usage is preserved though (multiple --addon flags), and that it errors out if an unknown addon is supplied. Help wanted!

[marcosinger]

@tstromberg I'm a newcomer here and I'd like to help with this one. I have some comments/questions before start:

to use a Slice instead of an Array, but I suspect there are devils in the implementation details

Accordingly to the docs, StringSliceVar might fix this issue without any drawbacks.

and that it errors out if an unknown addon is supplied

As far as I can see, both addon.Set and addon.RunCalbbacks functions handle with invalid addon name, but the command entry-point is addon.Start, which is filtering from invalid names. Do you think worth removing that check to keep the same behavior across commands?

WDYT?

Thanks!

[tstromberg]

@marcosinger - good find. I think you are on the right track. We aim to follow the principle of least surprise, so keeping the same behavior sounds correct. I agree that removing that filter would help.

One concern is that we should probably detect that the addons are invalid as soon as possible - certainly before starting the cluster, otherwise we may be wasting the users time.

[Aut0R3V]

@tstromberg I'd like to work on this.
/assign