Skip to content
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

Add helm_remote extension #19

Merged
merged 3 commits into from
Jun 5, 2020
Merged

Conversation

bobjackman
Copy link
Contributor

@bobjackman bobjackman commented Jun 2, 2020

@victorwuky
The currently recommended way of installing remote helm files is:

local("helm upgrade --install <chart> <repo>")

But this has a few shortcomings when running tilt down:

  1. the helm chart is not uninstalled
  2. any namespace created with helm's --create-namespace flag is not uninstalled (even when using tilt down --delete-namespaces

Another option is:

local("helm pull <repo>/<chart> --untar --destination helm_deps/<chart>")
k8s_yaml(helm("helm_deps/<chart>", namespace=myNamespace))

This has the advantage of solving bullet #1 above, but still has other shortcomings

  1. namespace must already exist in order to install there (helm() has no support for --create-namespace)
  2. even if namespace already exists, it still may not be cleaned up when running tilt down --delete-namespaces
  3. if the chart defines any CRDs, they won't be installed as part of your call to helm()

This extension solves all of the above. (see comments in code for a few technical caveats)

@drubin
Copy link
Contributor

drubin commented Jun 3, 2020

We currently also do some hacks as you have mentioned above. This would be super helpful thanks for turning it into a plugin!

Would it be possible to also add an README.md to the folder to show some usages/info?

Some nice examples

# avoid a namespace not found error
namespace_create(namespace) # do this early so it manages to register before we attempt to install into it

local('rm -rf helm_deps/%s' % repo_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires that helm_deps is git ignored right? Do you think we could download these into a temp directory location? or explicitly state these should be ignored.

This changes the current working directory which feels a little odd.

We currently do

local('if [ ! -d ./tmp/1.40.2/rabbitmq-ha ]; then helm fetch stable/rabbitmq-ha --untar --untardir ./tmp/1.40.2 --version 1.40.2; fi', quiet=QUIET)
rabbitMQYaml = helm('./tmp/1.40.2/rabbitmq-ha',

This allows us to cache and version lock the helm charts, but I have never loved that ./tmp was .gitignored but it was part of our setup.

Copy link
Contributor Author

@bobjackman bobjackman Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good thought about the .gitignore file. This definitely doesn't require that directory to be ignored, but I think it's probably a good idea to create a readme file for this sub-folder which suggests ignoring it.

As far as where the temp dir is located/what it's called. I'm 100% open to suggestions. I could also add an additional function for configuring the temp/cache location? Even so, I'm open to input on what a sane default might be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with it being documented and has sane defaults.

What about the versioning? Do you think your current method signature would support versioning? (this is something we would love to see and would love to switch to a proper plugin instead of our custom stuff)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drubin as I'm adding the ability to configure the temp/cache location, I'm seeing that your comment was maybe intended to refer to .tiltignore rather than .gitignore? In that case, yes, you're 100% correct that this dir would need to be ignored in order to prevent infinite recursive triggers to reprocess the tiltfile. So far I haven't been able to find a way around that, yet. Seems like tilt needs a native unwatch_file() function for situations like this, or to allow runtime ignoring of dynamic filenames.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobjackman
Copy link
Contributor Author

bobjackman commented Jun 3, 2020

@drubin Check out the latest commit

Markdown-processed version of readme here

…dditional function parameters.

* version -- specify the version of the chart to install
* username -- authenticate with a private helm repository
* password -- authenticate with a private helm repository
@@ -0,0 +1,42 @@
# Helm Remote
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rendered version of this file can be viewed here

@wu-victor
Copy link
Contributor

Thanks @kogi for the submission. Thank you @drubin for the review so far!

@jazzdan already assigned himself to do this PR and will review further on the Tilt side. Appreciate the collaboration so far!

Copy link
Contributor

@jazzdan jazzdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm @kogi the CRD installation seems to reliably fail for me. Here are my repro steps:

load('helm-remote.tilt', 'helm_remote')

helm_remote('memcached', repo_url='https://charts.bitnami.com/bitnami')
  1. tilt up
  2. Observe that CRD installation fails with error: the path "./.helm/memcached/latest/memcached/crds" does not exist
  3. Manually trigger the memcached-crds-install resource
  4. Observe that CRD installation fails with error: the path "./.helm/memcached/latest/memcached/crds" does not exist

Snapshot available here

Is that expected?

@bobjackman
Copy link
Contributor Author

@jazzdan nope, that's definitely not intended behavior. I'll dig in and see what's going on. Thanks!

@bobjackman
Copy link
Contributor Author

@jazzdan the latest commit should fix this

Copy link
Contributor

@jazzdan jazzdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot for contributing @kogi

@jazzdan jazzdan merged commit 357e589 into tilt-dev:master Jun 5, 2020
bobjackman pushed a commit to bobjackman/tilt-extensions that referenced this pull request Jun 8, 2020
* Add readme. Add ability to configure helm chart cache location. Add additional function parameters.

* version -- specify the version of the chart to install
* username -- authenticate with a private helm repository
* password -- authenticate with a private helm repository

* Fix script for helm charts that may not have any CRDs or charts that keep their CRDs in a non-standard location
@bobjackman bobjackman deleted the helm_remote branch June 12, 2020 20:52

# TODO: =====================================
# if it ever becomes possible for loaded files to also load their own extensions
# this method can be replaced by `load('ext://namespace', 'namespace_create')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants