double render the helmfile

[davidovich]

This allows using the values defined in the environments: section.

This is done by using a buffer instead of re-reading the file twice.

see #254

@mumoshu can you weigh in on the approach ?

[mumoshu]

Hey @davidovich, your solution seems very interesting!

I love how this makes helmfile.yaml cleaner and easier to read/write in case we want to reference environment values(not variables) in helmfile.yaml.

However, I'm a bit unsure if there's any bad side-effect of doing it in such way. I thought that helmfile would end up executing {{ readFile }} and {{ exec }} expressions in the template twice, that is costly in computation and io (in theory), but it doesn't sound critical to me.

So maybe there's nothing concrete that is preventing us from merging this, and closing #254 #297 in favor of this?

@Stono @osterman @sstarcher I'd appreciate your comments on this, too!

[sstarcher]

Seems like a plausible way to support the values directly.

[mumoshu]

I suspect if the nil as the first argument here makes any readFile call inside helmfile.yaml fail at first pass.

[mumoshu]

Fixed it in 2072ede

[mumoshu]

I've added some debug logs while fixing the readFile issue that would also help users.

[mumoshu]

One gotcha is that something like the below blows up helmfile on first-pass render with error calling eq: invalid type for comparison:

{{ if eq .Environment.Values.foo "bar" }}
...
{{ end }}

because the environment value isn't loaded yet hence nil.

A work-around is to modify it by adding a nil-check:

{{ if (.Environment.Values.foo) and eq .Environment.Values.foo "bar" }}
...
{{ end }}

[elemental-lf]

I thought that helmfile would end up executing {{ readFile }} and {{ exec }} expressions in the template twice, that is costly in computation and io (in theory), but it doesn't sound critical to me.

Could this lead to a difference in rendering if readFile or exec would return different information on the second call (think about a timestamp or a randomly generated password)? Some parts of the template using the value from the first call and some parts of the template using the value from the second call for example?

[davidovich]

Yes, if template functions have side effects, they would be executed twice. I am searching for a way to ignore, or make null operations for the first pass, as this first pass is only meant to parse the environment which is to be re-injected.

I was not aware of the new execution functions in template rendering, I will try to find a solution.

[sstarcher]

ya, due to the nature of certain functions this might not be a workable solution, unless for the first pass you overrode what the template functions do with mocks.

[davidovich]

@mumoshu I resolved errors and side-effects happening on the initial render. Also,

{{ if (.Environment.Values.foo) and eq .Environment.Values.foo "bar" }}
...
{{ end }}

is resolved. You can see this in main_test.go

[mumoshu]

Thanks @davidovich. It looks awesome!

One nit I found is that whether you should also stub readFile and exec for environment values file templates or not. For example, foo.yaml.gotmpl inside the environments.ENV.values array allows you to template environment values.

Not sure what's the less confusing way yet.

[sstarcher]

My overall concern is all of the edge cases this might bring up and any confusion it might cause people with how helmfile works. If everyone is comfortable with those things then I'm ok.

[davidovich]

One nit I found is that whether you should also stub readFile and exec for environment values file templates or not. For example, foo.yaml.gotmpl inside the environments.ENV.values array allows you to template environment values.

@mumoshu I am not sure I understand what you mean.

Note that if the prerender fails at all, it falls back to an empty Environment{Name: env, Values: map[string]interface{}} as before this change.

[mumoshu]

Also,

{{ if (.Environment.Values.foo) and eq .Environment.Values.foo "bar" }}
...
{{ end }}

is resolved. You can see this in main_test.go

@davidovich Hey! I took sometime to review it again, and noticed that you're testing the opposite of the issue I found.

It was that, the template execution fails when you had no values set in default/values.yaml . It happens because any missing value is considered a specially typed "" value in templates, so for example a comparison between the specially typed value and a string fails with type error error calling eq: invalid type for comparison.

You'll probably want to define a emptyValuesYaml := []byte{} instead of defaultValuesYalm and feed it to cover this case in your test.

[davidovich]

You'll probably want to define a emptyValuesYaml := []byte{} instead of defaultValuesYalm and feed it to cover this case in your test.

You are correct, this fails, but wouldn't it fail even if we didn't have a double render ? Is the concern related to less obvious failure mode ? I have added an err evaluation after the second render. we get the correct error there.

[mumoshu]

@davidovich Thanks for your continuous effort!

but wouldn't it fail even if we didn't have a double render ? Is the concern related to less obvious failure mode

Yeah, I agree. To provide you more context, @sstarcher and I had also concluded so #317.
Setting missingkey=error for every template used in helmfile makes sense to me now, so that the above example fails on missing key, rather than the non-obvious type error.

Does that sound good to you, too?

[mumoshu]

Setting missingkey=error for every template used in helmfile makes sense to me

Oops. In the context of double rendering, I suppose, this should be "set missingkey=zero for the first render, and missingkey=error for the second render".

Otherwise the first render against a helmfile that contains something like {{ if (.Environment.Values.foo) and eq .Environment.Values.foo "bar" }} would fail due to a missing key error.

[davidovich]

@mumoshu I have implemented the missingkey=error as you suggested.

I came back to this comment:

One nit I found is that whether you should also stub readFile and exec for environment values file templates or not.

and added a test to see the behavior. The current code as you wrote it supports nicely this usecase. If you have a .gotmpl file, all render functions are upheld and allow you to use readFile and exec in the environment section ! I think this is aligned with your expectations, and it makes a lot of sense.

[davidovich]

Tell me if everything is ok, and I will rebase on master.

[sstarcher]

I tested this off your branch and I had no issue with the double rendering. I'm having another issue with helmfile, but I don't think it's related to your rendering.

my rending in a helmfile is very simple so I'm confident your implementation will work for me.

[mumoshu]

LGTM 👍

I know there are still some concerns. But let's keep the ball rolling. @davidovich has done an awesome job that this is indeed working!

Thanks again for your contribution 🎉

[mumoshu]

This resolves #297

[mumoshu]

This seems to resolve #317, too.