-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(helm): add valueFiles field to specify custom value files #1099
Conversation
3501676
to
718010c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor comments, mostly regarding doc text.
key: some-value | ||
``` | ||
|
||
This will effectively create a new YAML with the supplies values and pass it to Helm when rendering/deploying the chart. This is particularly handy when you want to template in the values (see the next section for a good example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supplies => supplied
key: prod-value | ||
``` | ||
|
||
In this example, `some.key` is set to `"prod-value"` for the `prod` environment, and `other.key` keep the default value set in `values.default.yaml`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep => keeps. Or maybe better, maintains.
|
||
In this example, `some.key` is set to `"prod-value"` for the `prod` environment, and `other.key` keep the default value set in `values.default.yaml`. | ||
|
||
If you were to also set the `values` field in the Module configuration, the values there would take precedence over both of the value files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May drop the "hypothetical past tense" (or whatever it's called) and just say: "If you also set the values
field [...], the values there take..."
in \`valueFiles\`). | ||
`), | ||
valueFiles: joiArray(joi.string().posixPath({ subPathOnly: true })) | ||
.description(dedent` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be deline
? Noticed some extra new lines in the generated doc above (although not visible to when rendered as markdown).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that'll bunch up all the paragraphs, right? It's too much text for a single paragraph imo.
const chartPath = await getChartPath(module) | ||
const gardenValuesPath = getGardenValuesPath(chartPath) | ||
|
||
const valueFiles = module.spec.valueFiles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment saying that the gardenValuesPath
takes precedence over the other value files and is therefore appended last.
718010c
to
12c7ae5
Compare
Addressed the comments, to be squashed on merge. |
What this PR does / why we need it:
Previously you could only (easily) provide custom values to charts via the
values
key. Having different value files based on context/environment is a pretty common practice with Helm, so we should enable that.You could for example have a base values file with defaults, and then a custom one based on environment:
You can still use the current
values
field, which will take precedence over the files listed undervalueFiles
.Which issue(s) this PR fixes:
Closes #1098
Special notes for your reviewer:
See if the docs and the schema makes sense, as documented.
Does this PR introduce a user-facing change?:
Yes