tiltfile: helm builtin #994

jazzdan · 2019-01-11T18:20:19Z

No description provided.

jazzdan · 2019-01-11T18:20:31Z

.circleci/Dockerfile

@@ -5,3 +5,10 @@ RUN go get -u github.com/kisielk/errcheck
 RUN go get -u github.com/google/wire/cmd/wire

 RUN go get -u sigs.k8s.io/kustomize
+
+USER root


is this a cool thing to do?

jazzdan · 2019-01-11T18:21:31Z

internal/tiltfile/tiltfile_state.go

@@ -15,6 +15,12 @@ import (
 	"github.com/windmilleng/tilt/internal/model"
 )

+// TODO(dmiller) this needs to add all of the files in the directory as dependencies


Should we have a add_dir_as_dependencies function? Or is this a sign that we shouldn't be trying to implement this in Starlark, and should implement it in Go so that we can walk that chart directory and record dependencies?

I think we need to have lots of thoughts about dependencies, and shouldn't block this change on that.

Are you sure that's the right way to get deps from helm? I don't know enough about it to know how else it could have deps. What does skaffold do?

dbentley

It would help me to review the user experience with the docs that describe this.

jazzdan · 2019-01-11T18:24:07Z

@dbentley will do after lunch. In the meantime here is a test if it helps.

nicks

cool!

nicks · 2019-01-11T19:07:46Z

internal/tiltfile/tiltfile_state.go

+// TODO(dmiller) this needs to add all of the files in the directory as dependencies
+const helmFunc = `
+def helm(path):
+	return local("helm template " + path)


do we also need a way to mark the path as read, so it registers as a config file?

Yeah see TODO, and @dbentley's comment.

nicks · 2019-01-11T19:09:23Z

internal/tiltfile/tiltfile_state.go

+	r = addBuiltin(r, readFileN, s.skylarkReadFile)
+
+	// TODO(dmiller) this really only needs to be done once, on startup
+	r, err := starlark.ExecFile(thread, "helm.builtin", helmFunc, r)


can we use a dummy thread here? i don't see a good reason to pass in an external thread, do you?

also probably don't need to re-exec this every time we exec a new tiltfile

nicks · 2019-01-11T19:10:37Z

internal/tiltfile/tiltfile_test.go

+	f.setupHelm()
+
+	f.file("Tiltfile", `
+yml = helm('helm')


can you also add a test that this works with git_repo('.').path('helm')?

jazzdan · 2019-01-11T21:09:45Z

@dbentley added docs

dbentley · 2019-01-11T21:14:24Z

docs/write_your_tiltfile.md

@@ -18,6 +18,9 @@ k8s_yaml(['foo.yaml', 'bar.yaml'])
 # run kustomize to generate yaml
 k8s_yaml(kustomize('config_dir'))

+# run helm to generate yaml
+k8s_yaml(helm('chart_dir'))
+
 # run a custom command to generate yaml


Can we move local (the general case) above the specific cases (helm and kustomize), and then maybe a comment like "we also handle common tools to generate yaml"?

Yeah totally

jazzdan · 2019-01-11T22:17:54Z

internal/tiltfile/tiltfile.go

@@ -34,9 +34,6 @@ func Load(ctx context.Context, filename string, matching map[string]bool) (manif
 	tfRoot, _ := filepath.Split(absFilename)

 	s := newTiltfileState(ctx, absFilename, tfRoot)
-	defer func() {


this doesn't seem to do anything, but maybe it has something to do with named returns that I'm not understanding?

seems to work ok for me
https://play.golang.org/p/N2e7s_wsABh

can you give a specific example?

but we're returning s.configFiles so I don't understand why this is necessary? "It doesn't do anything" was a poor choice of words. It seems superfluous.

if there's an error, i think this function needs to return non-nil? i can write a test for it to double-check the behavior if you want.

Ohhh I think I get it now. Okay I'll add it back.

jazzdan · 2019-01-11T22:19:30Z

fyi @nicks I replaced the python implementation with a Go implementation to make tracking dependencies easier. It was a good experiment and we'll need to support things like this in the future for when people write Tilt libraries but I decided it wasn't worth the effort for now.

nicks · 2019-01-11T22:23:54Z

.circleci/config.yml

-      - run: echo 'export PATH="/usr/local/opt/go@1.10/bin:$PATH"' >> $BASH_ENV
+          key: homebrew_cache_v8
+      - run: echo 'export PATH="/usr/local/opt/go@1.11/bin:$PATH"' >> $BASH_ENV
+      - run: curl --silent --show-error --location --fail --retry 3 --output /tmp/helm.tar.gz  https://storage.googleapis.com/kubernetes-helm/helm-v2.12.1-darwin-amd64.tar.gz &&


can you add a comment on why you're not using homebrew here? this seems very brittle

Yes, can do. Basically it looks like homebrew isn't being updated any more, it has a much older verison of helm that actually causes the tests to fail in a minor way (just a capitalization difference).

nicks · 2019-01-11T22:25:14Z

internal/tiltfile/files.go

+		return nil, fmt.Errorf("Argument 0 (path): %v", err)
+	}
+
+	cmd := fmt.Sprintf("helm template %s", path)


this is going to have major escaping problems because you're passing a thing directly to shell here. can you express this as an argv, i.e., []string{"helm", "template", path}

nicks · 2019-01-11T22:25:58Z

internal/tiltfile/files.go

+		if err != nil {
+			return fmt.Errorf("error accessing path (%s): %v", path, err)
+		}
+		if !info.IsDir() {


maybe use IsReg() to avoid catching tempfile symlinks and sockets?

nicks · 2019-01-11T22:26:53Z

internal/tiltfile/files.go

+
+	deps := []string{}
+
+	err = filepath.Walk(localPath.path, func(path string, info os.FileInfo, err error) error {


i feel like our watcher library has much more efficient ways of watching directories so that we don't need to do a Walk. Where is this falling down?

Yep this is a good point, let's just add the whole directory as a "configFile". Seems to work just fine.

jazzdan requested review from dbentley and nicks January 11, 2019 18:20

jazzdan commented Jan 11, 2019

View reviewed changes

dbentley reviewed Jan 11, 2019

View reviewed changes

nicks approved these changes Jan 11, 2019

View reviewed changes

jazzdan force-pushed the dmiller/ch1230/add-helm-primitive-to-tiltfile branch from 3b37186 to f87c12e Compare January 11, 2019 19:56

dbentley reviewed Jan 11, 2019

View reviewed changes

jazzdan force-pushed the dmiller/ch1230/add-helm-primitive-to-tiltfile branch from 0890a65 to 472146a Compare January 11, 2019 21:27

jazzdan commented Jan 11, 2019

View reviewed changes

nicks reviewed Jan 11, 2019

View reviewed changes

Dan Miller added 17 commits January 11, 2019 18:07

tiltfile: helm builtin

116057d

update circle ci image

a6ecb25

bump homebrew cache

de7129a

update macos installation

8a0efb6

use dummy thread

245ce33

Add test for repo path

8e1eee5

memoize builtins

04ac54c

fix macos helm install

469a13e

remove obsolute comment

4120cc5

add docs

9c67001

addBuiltin doesn't need to be its own function

a337d86

update docs

0f0bc1f

Implement it in golang so we can get dependencies

99f9ed4

golang implementation of helm, dependency support

6039de3

use argv

d1d0f34

just add the directory as a dependency rather than walking the files

04c2c6e

add back the defer

8508865

add back teardown

3f4b0ae

jazzdan force-pushed the dmiller/ch1230/add-helm-primitive-to-tiltfile branch from 3640923 to 3f4b0ae Compare January 11, 2019 23:08

nicks approved these changes Jan 14, 2019

View reviewed changes

jazzdan merged commit fbf167b into master Jan 14, 2019

nicks deleted the dmiller/ch1230/add-helm-primitive-to-tiltfile branch March 31, 2021 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tiltfile: helm builtin #994

tiltfile: helm builtin #994

jazzdan commented Jan 11, 2019

jazzdan Jan 11, 2019

nicks Jan 11, 2019

jazzdan Jan 11, 2019

dbentley Jan 11, 2019

dbentley left a comment

jazzdan commented Jan 11, 2019

nicks left a comment

nicks Jan 11, 2019

jazzdan Jan 11, 2019

nicks Jan 11, 2019

nicks Jan 11, 2019

nicks Jan 11, 2019

jazzdan commented Jan 11, 2019

dbentley Jan 11, 2019

jazzdan Jan 11, 2019

jazzdan Jan 11, 2019

nicks Jan 11, 2019

jazzdan Jan 11, 2019

nicks Jan 11, 2019

jazzdan Jan 11, 2019

jazzdan commented Jan 11, 2019

nicks Jan 11, 2019

jazzdan Jan 11, 2019

nicks Jan 11, 2019

nicks Jan 11, 2019

nicks Jan 11, 2019

jazzdan Jan 11, 2019


		deps := []string{}

		err = filepath.Walk(localPath.path, func(path string, info os.FileInfo, err error) error {

tiltfile: helm builtin #994

tiltfile: helm builtin #994

Conversation

jazzdan commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbentley left a comment

Choose a reason for hiding this comment

jazzdan commented Jan 11, 2019

nicks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jazzdan commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jazzdan commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment