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 ValuesFiles to HelmChart spec #305

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Add ValuesFiles to HelmChart spec #305

merged 1 commit into from
Apr 19, 2021

Conversation

arbourd
Copy link
Contributor

@arbourd arbourd commented Mar 8, 2021

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

Closes #291

@arbourd
Copy link
Contributor Author

arbourd commented Mar 8, 2021

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.

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this @arbourd 🥇

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

api/v1beta1/helmchart_types.go Outdated Show resolved Hide resolved
api/v1beta1/helmchart_types.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
api/v1beta1/helmchart_types.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
api/v1beta1/helmchart_types.go Outdated Show resolved Hide resolved
@relu
Copy link
Member

relu commented Mar 10, 2021

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

@hiddeco hiddeco added area/helm Helm related issues and pull requests enhancement New feature or request labels Mar 10, 2021
@arbourd arbourd requested review from relu and hiddeco March 16, 2021 23:31
@arbourd
Copy link
Contributor Author

arbourd commented Apr 4, 2021

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

@relu
Copy link
Member

relu commented Apr 6, 2021

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

controllers/helmchart_controller_test.go Outdated Show resolved Hide resolved
@relu relu mentioned this pull request Apr 6, 2021
@arbourd
Copy link
Contributor Author

arbourd commented Apr 7, 2021

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

@arbourd arbourd requested a review from relu April 9, 2021 16:56
@arbourd arbourd marked this pull request as ready for review April 10, 2021 03:14
Comment on lines +407 to +403
if v == "values.yaml" {
valuesMap = transform.MergeMaps(valuesMap, helmChart.Values)
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note this @relu

@arbourd
Copy link
Contributor Author

arbourd commented Apr 11, 2021

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
Copy link
Member

hiddeco commented Apr 12, 2021

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
Copy link
Contributor Author

arbourd commented Apr 15, 2021

@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
Copy link
Member

relu commented Apr 15, 2021

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
Copy link
Contributor Author

arbourd commented Apr 15, 2021

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).

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

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

Signed-off-by: Dylan Arbour <arbourd@users.noreply.github.com>
@arbourd
Copy link
Contributor Author

arbourd commented Apr 19, 2021

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

Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

All good from my side as well 🙌

@hiddeco hiddeco merged commit f56c96f into fluxcd:main Apr 19, 2021
@hiddeco
Copy link
Member

hiddeco commented Apr 19, 2021

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 arbourd deleted the values-files branch April 19, 2021 13:34
@arbourd
Copy link
Contributor Author

arbourd commented Apr 19, 2021

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

@hiddeco
Copy link
Member

hiddeco commented Apr 19, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple valuesFiles in HelmChart source
4 participants