-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(manager/kustomize): Support inflating helm charts #34277
base: main
Are you sure you want to change the base?
Conversation
const chartsDeletion = status?.deleted ?? []; | ||
|
||
const fileChanges: UpdateArtifactsResult[] = []; | ||
await writeLocalFile(packageFileName, newPackageFileContent); |
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.
Doesn't make sense to write a file to disk here. What's the intention?
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.
Just inexperience with the project, i wasn't sure if the new packagefile content needed to be written to disk here or if it's done somewhere else.
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.
you need to do it before calling helm, otherwise it'll install the old version again
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.
It looks like we don't need to write newPackageFileContent
in the artifacts management.
I'm running 3f05793 now and the updated kustomization.yaml
file is included in the PR without me having to write it in artifacts management, see this PR for example: https://github.com/micke/renovate-kustomize-artifacts/pull/8/files#diff-5c38001a592677b709f1e7de7702d683403356ce7824481d04ed5def445663ba
If that's not by design or if you think we should write it anyways then i'll add it back but we certainly don't need to write it before calling helm as we're not using the file to pull the helm charts...
|
||
function generateHelmEnvs(): ExtraEnv { | ||
return { | ||
HELM_EXPERIMENTAL_OCI: '1', |
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.
do we really need this? should only be set if user sets contraints which causes a helm version <3.8.0
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.
I took a lot of inspiration from how renovate handles helm in the helmv3
manager. I wasn't sure what's the best way to reason here, i settled for being consistent with how helm was configured.
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.
Let me know if you think i should remove it, otherwise i'm under the impression that it should be there as-is.
Co-authored-by: Michael Kriese <[email protected]>
Co-authored-by: Michael Kriese <[email protected]>
I've run this on a repository with two example kustomization.yaml files, one with "regular" helm charts and one with OCI chart: https://github.com/micke/renovate-kustomize-artifacts The resulting "regular" helm chart PR: micke/renovate-kustomize-artifacts#15 A side effect of having the helm charts inflated is that renovate manages updates for sub-charts and images in these inflated charts. |
Would be great to get another review on this! |
Changes
helmGlobals
options from kustomization files to support custom destination for inflated helm charts.Context
Closes #14137
The helm charts will be inflated if any of the following is true:
currentVersion
is inflated, ORkustomizeInflateHelmCharts
postUpdateOptions
is enabled.If the
currentVersion
is inflated then they will be deleted.If the chart is already inflated then Renovate won't try to inflate them again.
I decided to accomplish this by directly interfacing with helm commands to inflate the charts by calling
helm add
instead of relying on usingkustomize build
to have kustomize inflate the charts.I took this approach because i've seen setups where a kustomization might need pre-processing to be able to be built, a common example of this might be encrypted secrets, where the secrets referenced in the kustomization need to be decrypted before using
kustomize build
.I've tried to structure the code so that it would work when a dependency is upgraded but i also envision a scenario where Renovate could be used to inflate existing charts used in kustomize kind of like how Renovate today can generate a PR to pin docker digests with a "Pin dependencies" PR when using
docker:pinDigests
.But i'm not yet comfortable enough with the codebase to dive deeper into what it would take to make that a possibility, so i would greatly appreciate guidance on that.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: