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

Helm module not apply custom values files over chart defaults. #1642

Closed
davidjeddy opened this issue Feb 25, 2020 · 10 comments
Closed

Helm module not apply custom values files over chart defaults. #1642

davidjeddy opened this issue Feb 25, 2020 · 10 comments

Comments

@davidjeddy
Copy link
Contributor

Bug

Per #1638 the Helm module now includes custom_values.yaml into the ./.garden/build/{module name}/ directory. After deploying the module values from said custom_values.yaml are not applied over the Helm charts default values.

Current Behavior

Custom Helm chart values not applied over default values.

Expected behavior

Overwrite Helm chart default values w/ specified custom values

Reproducible example

  • Using MariaDbb-Galera cluster
  • Create custom values.yaml
  • Include custom values.yaml in Garden module
  • Build and deploy

Screenshot from 2020-02-25 11-37-23

Workaround

none known

Suggested solution(s)

Apply custom values.yaml to .rendered.yaml AFTER the default chart values have been rendered

Additional context

n/a

Your environment

❯ garden version &&
> kubectl version &&
> docker version
0.11.4
Client Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.0", GitCommit:"ddf47ac13c1a9483ea035a79cd7c10005ff21a6d", GitTreeState:"clean", BuildDate:"2018-12-03T21:04:45Z", GoVersion:"go1.11.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"14+", GitVersion:"v1.14.10-gke.17", GitCommit:"bdceba0734835c6cb1acbd1c447caf17d8613b44", GitTreeState:"clean", BuildDate:"2020-01-17T23:10:13Z", GoVersion:"go1.12.12b4", Compiler:"gc", Platform:"linux/amd64"}
Client: Docker Engine - Community
 Version:           19.03.6
 API version:       1.40
 Go version:        go1.12.16
 Git commit:        369ce74a3c
 Built:             Thu Feb 13 01:27:49 2020
 OS/Arch:           linux/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          19.03.6
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.12.16
  Git commit:       369ce74a3c
  Built:            Thu Feb 13 01:26:21 2020
  OS/Arch:          linux/amd64
  Experimental:     true
 containerd:
  Version:          1.2.10
  GitCommit:        b34a5c8af56e510852c35414db4c1f4fa6172339
 runc:
  Version:          1.0.0-rc8+dev
  GitCommit:        3e425f80a8c931f88e6d94a8c831b9d5aa481657
 docker-init:
  Version:          0.18.0
  GitCommit:        fec3683

Thank you for the time and effort, much appreciated.

@edvald
Copy link
Collaborator

edvald commented Feb 25, 2020

Thanks for the detailed report @davidjeddy! Love the screen grab :) We'll take a look.

@edvald
Copy link
Collaborator

edvald commented Feb 25, 2020

This is actually how it's meant to work already... I wonder what's failing here. How are you specifying the valueFiles field?

@davidjeddy
Copy link
Contributor Author

In this use case we are not using valueFiles. Only the include: [...].

@edvald
Copy link
Collaborator

edvald commented Feb 28, 2020

Hey @davidjeddy. So, I was trying to reproduce this, and then it occurred to me to check whether the intended/designed behavior matches your expectations.

The precedence order is as follows (in increasing order of precedence, i.e. last is preferred):

  1. The chart default values.yaml file
  2. Any files specified in valueFiles in the garden.yml
  3. The values specified on the values field in garden.yml

As far as I can tell, that's working as expected. If that's not what you're finding, it'd be useful to see the garden.yml to see what's going on.

@edvald
Copy link
Collaborator

edvald commented Feb 28, 2020

Perhaps the confusion relates to using the filename values.yaml, because that is what Helm uses by default, and therefore has the lowest precedence.

@davidjeddy
Copy link
Contributor Author

davidjeddy commented Mar 2, 2020

The order of operation makes sense, exactly the same order as CSS or any other inheritance system.

As per the screen capture reference_values.yaml is our custom values config we wish to over ride the Helm charts default values with. However, it did not appear to be applied as the image version is that of the default values.yaml; not the reference.yaml we configured.

I have attached the Helm modules garden.yaml for your inspection.

# The schema version of this module's config (currently not used).
apiVersion: garden.io/v0

# What kind of Garden module
kind: Module

# The type of this module.
type: helm

# The name of this module.
name: mariadb-galera-module

# A description of the module.
description: Created 2 Galera clusters w/ 3 compute instances each. Federate the contents of the clusters to provide a single SQL storage solution.

# Specify a list of POSIX-style paths or globs that should be regarded as the source files for this module. Files that
# do *not* match these paths or globs are excluded when computing the version of the module, when responding to
# filesystem watch events, and when staging builds.
include: ["reference_values.yaml"]

# A valid Helm chart name or URI (same as you'd input to `helm install`). Required if the module doesn't contain the
# Helm chart itself.
chart: mariadb-galera

# The path, relative to the module path, to the chart sources (i.e. where the Chart.yaml file is, if any). Not used
# when `base` is specified.
chartPath: .

# List of names of services that should be deployed before this chart.
# dependencies: []

# Deploy to a different namespace than the default one configured in the provider.
namespace: mariadb-galera

# Optionally override the release name used when installing (defaults to the module name).
releaseName: mariadb-galera-release

# The repository URL to fetch the chart from.
repo: https://charts.bitnami.com/bitnami

# Time in seconds to wait for Helm to complete any individual Kubernetes operation (like Jobs for hooks).
timeout: 300

# The chart version to deploy.
version: 0.8.1

# Specify value files to use when rendering the Helm chart. These will take precedence over the `values.yaml` file
# bundled in the Helm chart, and should be specified in ascending order of precedence. Meaning, the last file in
# this list will have the highest precedence.
#
# If you _also_ specify keys under the `values` field, those will effectively be added as another file at the end
# of this list, so they will take precedence over other files listed here.
#
# Note that the paths here should be relative to the _module_ root, and the files should be contained in
# your module directory.
# valueFiles: [
#   ./reference_values.yaml
# ]

We also tried the following but it has a known issue that should be fixed as part of the next Garden release. I wanted to provide it here for reference.

# The schema version of this module's config (currently not used).
apiVersion: garden.io/v0

# What kind of Garden module
kind: Module

# The type of this module.
type: helm

# The name of this module.
name: mariadb-galera-module

# A description of the module.
description: Created 2 Galera clusters w/ 3 compute instances each. Federate the contents of the clusters to provide a single SQL storage solution.

# Specify a list of POSIX-style paths or globs that should be regarded as the source files for this module. Files that
# do *not* match these paths or globs are excluded when computing the version of the module, when responding to
# filesystem watch events, and when staging builds.
# include: []

# A valid Helm chart name or URI (same as you'd input to `helm install`). Required if the module doesn't contain the
# Helm chart itself.
chart: mariadb-galera

# The path, relative to the module path, to the chart sources (i.e. where the Chart.yaml file is, if any). Not used
# when `base` is specified.
chartPath: .

# List of names of services that should be deployed before this chart.
# dependencies: []

# Deploy to a different namespace than the default one configured in the provider.
namespace: mariadb-galera

# Optionally override the release name used when installing (defaults to the module name).
releaseName: mariadb-galera-release

# The repository URL to fetch the chart from.
repo: https://charts.bitnami.com/bitnami

# Time in seconds to wait for Helm to complete any individual Kubernetes operation (like Jobs for hooks).
timeout: 300

# The chart version to deploy.
version: 0.8.1

# Specify value files to use when rendering the Helm chart. These will take precedence over the `values.yaml` file
# bundled in the Helm chart, and should be specified in ascending order of precedence. Meaning, the last file in
# this list will have the highest precedence.
#
# If you _also_ specify keys under the `values` field, those will effectively be added as another file at the end
# of this list, so they will take precedence over other files listed here.
#
# Note that the paths here should be relative to the _module_ root, and the files should be contained in
# your module directory.
valueFiles: [
  ./reference_values.yaml
]

@edvald
Copy link
Collaborator

edvald commented Mar 2, 2020

Ah now I see. So, the issue that we fixed (#1637) was that when you specified valueFiles they weren't automatically included in the include field. That's been fixed and released, so you just need to specify the valueFiles  field, but you needed to specify that one either way, otherwise the file won't be picked up. Let me know if that does the trick.

@davidjeddy
Copy link
Contributor Author

If I understand correctly both include: [] AND valueFiles: [] must be set in order to over ride Helm Chart default values?

@davidjeddy
Copy link
Contributor Author

Ok, got it to work. For anyone who finds this later:

# for any and all files that should be considered part of the module
include: [
  reference_values.yaml
]
...
# HELM value overrides
valueFiles: [
  reference_values.yaml
]

So the Helm value over ride must first be part of the module (include) AND stated as the over ride (valueFiles).

This now makes sense to me. 💡

@edvald
Copy link
Collaborator

edvald commented Mar 5, 2020

Glad it worked out! As of next release the additional include field is not necessary btw, that was a bug on our side.

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

No branches or pull requests

2 participants