-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable or disable addons per profile #6124
Enable or disable addons per profile #6124
Conversation
However, this means that addon information won't persist beyond a 'minikube delete'
/ok-to-test |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: priyawadhwa 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:
Approvers can indicate their approval by writing |
Error: running mkcmp: exit status 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Two minor notes.
Remove old TODO and remove function to get current profile as it can create errors when trying to get multiple profiles.
Codecov Report
@@ Coverage Diff @@
## master #6124 +/- ##
==========================================
+ Coverage 38.35% 38.36% +<.01%
==========================================
Files 120 122 +2
Lines 8103 8143 +40
==========================================
+ Hits 3108 3124 +16
- Misses 4591 4606 +15
- Partials 404 413 +9
|
All Times minikube: [ 122.713917 122.950899 121.816684] Average minikube: 122.493833 Averages Time Per Log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good work ! I would just add in the PR description a paste of the new config format just for clarity.
nicely done, just tested on my machine it works great. here is the profile config after this PR.
for the second profile without any addons: { "Name": "p2", "KeepContext": false, "EmbedCerts": false, "MinikubeISO": "https://storage.googleapis.com/minikube/iso/minikube-v1.6.0.iso", "Memory": 2000, "CPUs": 2, "DiskSize": 20000, "VMDriver": "hyperkit", "ContainerRuntime": "docker", "HyperkitVpnKitSock": "", "HyperkitVSockPorts": [], "DockerEnv": null, "InsecureRegistry": null, "RegistryMirror": null, "HostOnlyCIDR": "192.168.99.1/24", "HypervVirtualSwitch": "", "KVMNetwork": "default", "KVMQemuURI": "qemu:///system", "KVMGPU": false, "KVMHidden": false, "DockerOpt": null, "DisableDriverMounts": false, "NFSShare": [], "NFSSharesRoot": "/nfsshares", "UUID": "", "NoVTXCheck": false, "DNSProxy": false, "HostDNSResolver": true, "KubernetesConfig": { "KubernetesVersion": "v1.17.0", "NodeIP": "192.168.64.191", "NodePort": 8443, "NodeName": "minikube", "APIServerName": "minikubeCA", "APIServerNames": null, "APIServerIPs": null, "DNSDomain": "cluster.local", "ContainerRuntime": "docker", "CRISocket": "", "NetworkPlugin": "", "FeatureGates": "", "ServiceCIDR": "10.96.0.0/12", "ImageRepository": "", "ExtraOptions": null, "ShouldLoadCachedImages": true, "EnableDefaultCNI": false }, "HostOnlyNicType": "virtio", "NatNicType": "virtio", "Addons": null } |
one thing that could be for a PR is output of addons list it used to show list and status for all profiles, now we should be clear that if u wanna see addon list, u need to pass -p I am totally okay with having it in following PRs. Current output:
|
@medyagh the output of addons list should also take the In addons_list.go we call addonBundle.IsEnabled() and in this PR I updated |
TestAddons failed:
Check that the |
All Times Minikube (PR 6124): [ 122.765268 123.385578 122.021202] Average Minikube (PR 6124): 122.724016 Averages Time Per Log
|
All Times minikube: [ 122.818927 121.265982 125.513654] Average minikube: 123.199521 Averages Time Per Log
|
All Times minikube: [ 127.417149 124.653175 123.807327] Average minikube: 125.292550 Averages Time Per Log
|
All Times minikube: [ 122.351220 123.394254 123.076054] Average minikube: 122.940509 Averages Time Per Log
|
…failing on virtualbox
All Times minikube: [ 120.435942 122.476699 120.827028] Average minikube: 121.246556 Averages Time Per Log
|
weird to see "verify_cache_inside_node" test fail ! I wonder how could that be related to this PR |
/retest this please |
All Times minikube: [ 124.561060 125.265181 116.068183] Average minikube: 121.964808 Averages Time Per Log
|
All Times minikube: [ 128.123514 127.219793 126.353198] Average minikube: 127.232168 Averages Time Per Log
|
All Times Minikube (PR 6124): [ 122.857389 126.527433 124.566560] Average minikube: 119.619963 Averages Time Per Log
|
@@ -33,7 +33,7 @@ import ( | |||
"github.com/pkg/errors" | |||
"github.com/spf13/cobra" | |||
"github.com/spf13/viper" | |||
configcmd "k8s.io/minikube/cmd/minikube/cmd/config" | |||
pkgaddons "k8s.io/minikube/pkg/addons" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be made just "addons" (here, and elsewhere when possible)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the compiler complains that there's a constant named addons in the same package :(
add better error handling and remove duplication in enableOrDisableAddonsInternal
All Times minikube: [ 127.516988 129.103127 128.002131] Average minikube: 128.207416 Averages Time Per Log
|
/retest |
/ok-to-test |
Thank you! |
This PR makes a few changes to the addons code:
It moves a lot of the addons code for enabling/disabling addons from package
cmd/config/util
to a new packageminikube/pkg/addons
We now enable/disable addons per profile, and so addons information is stored in machine configs per profile. Any addon related information in the global config is now disregarded.
Since addons can no longer be globally saved, on
minikube delete
the state of addons is wipedThis PR should fix #4227
The new config has an
Addons
section: