-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Addons: helm based addons #21847
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
base: master
Are you sure you want to change the base?
Addons: helm based addons #21847
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: volatilemolotov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @volatilemolotov. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
|
Can one of the admins verify this patch? |
pkg/addons/helm.go
Outdated
| "k8s.io/minikube/pkg/minikube/vmpath" | ||
| ) | ||
|
|
||
| func helmCommand(ctx context.Context, chart *assets.HelmChart, enable bool) *exec.Cmd { |
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.
add a comment explain in short what this func does?
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.
Yeah, short description added
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.
how about make this a Generic func, and have two other helpers
called
insatallHelmChart
uninstallHelmChart
so we dont have to pass the enable to it
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.
Added installHelmChart and uninstallHelmChart along with helmUninstallOrInstall
pkg/addons/helm.go
Outdated
| default: | ||
| return fmt.Errorf("failure to detect architecture or unsupported architecture: %s", arch) | ||
| } | ||
| helmURL := fmt.Sprintf("https://get.helm.sh/helm-v3.19.0-linux-%s.tar.gz", helmArch) |
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.
is there a "latest" helm url that we could use ?
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.
Not aware of any available latest target.
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.
changed this to get_helm.sh
pkg/addons/helm.go
Outdated
| func helmInstall(addon *assets.Addon, runner command.Runner) error { | ||
| _, err := runner.RunCmd(exec.Command("test", "-f", "/usr/bin/helm")) | ||
| if err != nil { | ||
| // If not, install it |
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.
how about installing it through their Script ?
https://helm.sh/docs/intro/install#from-script
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 will install latest by script and no need for us to parse archs
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.
Did not want to shoehorn a whole script into this but we could do that. It would require a chmod call and similar but doable. Ill see if its works
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.
Actually not possible without changing either ISO or the script. Script will try to put helm into /usr/local/bin which the VM does not have. Issue encountered on macos with krunkit driver. Same happens on linux with kvm driver
Output from minikube ssh and $ ls /usr/
bin lib lib64 libexec sbin share
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.
Ill revisit this one. Probably we can make the dir and copy the binary where it needs to be and satisfy both environments
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.
Decided to go with checking if the folder exists and create it if it does not. Then copy helm binary to the PATH.
pkg/addons/addons.go
Outdated
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| defer cancel() | ||
| cmd := helmCommand(ctx, addon.HelmChart, enable) |
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.
would this ever be called with disable?
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.
Yes
| } | ||
| if strings.HasSuffix(fPath, ".yaml") { | ||
| deployFiles = append(deployFiles, fPath) | ||
| } |
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.
we can add a documentation to the website https://minikube.sigs.k8s.io/docs/contrib/addons/
we can add a block (how to add helm based 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.
Pushed a new site doc with some explanation. As we work on adding a actual addon we will update the doc for the helm based addon
This PR adds support for addons based on helm.
Adds the following:
Example impelentation:
In addons list in pkg/minikube/assets/addons.go
Provides foundation for #21257