Add ValuesFiles to HelmChart spec

[arbourd]

Allows the use of more than values file to be declared and used in a HelmRelease, HelmChart.

Closes #291

[arbourd]

I can recreate this test failure locally at least. I'll need to invest some time on getting that suite up and running properly so I can see what is causing the, I assume, CrashLoopBackOff.

[hiddeco]

Thank you for submitting this @arbourd 🥇

Did a quick first round of review with some minor comments.

[relu]

Nice work so far, @arbourd! I like the idea of having the ability to use multiple override values files combined.

[arbourd]

@relu and @hiddeco: I will need your help understanding these tests please 🙏

[relu]

I'm looking into this and will get back to you if I can figure it out.

[arbourd]

Because the test suite is not working this PR is now blocked by #332

[arbourd]

Default values are not available from the filesystem, only via helmChart.Values

[arbourd]

Please note this @relu

[arbourd]

Okay so finally this passes all tests, for both Git repositories and Helm repositories.

As I was working on it last night, I had a realization that way we handle default values is problematic. I think we should always merge the default values. For most Helm users, helm install -f values-prod.yaml naturally implies that it will be merged with the default values. Flux does this differently, noting that it won't. This behaviour also diverges from other CloudNative CD platforms like ArgoCD, where valuesFiles represents a list of overrides.

I think that if someone is expecting a fully-declarative (no merging) values override they should implement the best practise of duplicating it exactly (as Bitnami used to do for example).

@hiddeco, @relu -- thoughts?

[hiddeco]

As I was working on it last night, I had a realization that way we handle default values is problematic. I think we should always merge the default values. For most Helm users, helm install -f values-prod.yaml naturally implies that it will be merged with the default values. Flux does this differently, noting that it won't.

This behavior was chosen because we also have a ValuesFrom on the HelmRelease that behaves more like the -f flag from Helm, but I can (now) see how this from a user's perspective doesn't make much sense.

No strong opinion on (not) changing this, so would welcome @relu's thoughts.

[arbourd]

@relu, any thoughts on this?

Also, @relu and @hiddeco are there any outstanding issues other than that discussion that are left on this PR? Would love to see this in the next release if possible. 😍

[relu]

The way I see it is that ValuesFrom on the HelmRelease resource refers to overrides (like you would use different values in different environments), while ValuesFiles on HelmChart would be used to package the chart with some defaults that may or may not be overridden in the HelmRelease using it. Considering this, I would think ideally we should leave it up to the user to decide what goes in or not into the re-packaged chart, essentially enabling the user to fully replace the default values.yaml.

@arbourd you mentioned that Argo and other tools are doing it differently, I don't think that's the case. Argo also has the ability to supply multiple values files at the level of the release, while we support that as well since ValuesFrom on the HelmRelease` resource accepts a list of override files.

[arbourd]

We use ArgoCD in production right now and this what the production application CRD looks like:

spec:
  destination:
    namespace: default
  source:
    path: ./path-to-chart
    repoURL: git@github.com:org/repo.git
    targetRevision: HEAD
    helm:
      valueFiles:
        - values-production.yaml

The default values are implicitly included in the helm template command that ArgoCD calls.

The trade off for this additional flexibility (being able to exclude the default values) is a difference in expectations from Helm itself, which may lead to confusion from Helm users new to Flux.

That being said, I also don't have a strong opinion on this. I brought it up on this PR because if we were to change it, this would be the time (already changing the spec).

[hiddeco]

This looks OK to me, thank you very much @arbourd 🥇

[arbourd]

Let me know if you need anything else to make it more than OK 😁

[relu]

All good from my side as well 🙌

[hiddeco]

There are two additional things @arbourd:

1. The field must be added to the template in the helm-controller API so that it can be configured by users there.
2. It should probably be documented on the website and in the migration guide, this documentation lives (for now) in fluxcd/flux2.

[arbourd]

Do we need to prerelease of the source-controller for that @hiddeco?

[hiddeco]

@arbourd you can create a PR that works with an API dependency from main, then change it to a released version once it exists.