Prefer helm dependency build over dependency up when Chart.lock exists#185
Open
sstarcher wants to merge 1 commit into
Open
Prefer helm dependency build over dependency up when Chart.lock exists#185sstarcher wants to merge 1 commit into
helm dependency build over dependency up when Chart.lock exists#185sstarcher wants to merge 1 commit into
Conversation
Author
|
This won't work due to chartify rewriting Chart.yaml which would invalidate the lock |
Author
|
This looks to be working if in helmfile we also update the Chart.lock's SHA when we rewrite the path |
…xists When chartify is engaged (e.g. helmfile invokes it for releases that use jsonPatches, strategic-merge patches, or kustomize), it copies the chart to a temp directory and runs `helm dependency up` to flatten dependencies before rendering. `dep up` re-resolves from Chart.yaml's version constraints and rewrites Chart.lock, which silently pulls newer dependency versions whenever constraints permit (e.g. `version: "*"`). This diverges from helmfile's non-chartify code path (pkg/helmexec/exec.go `BuildDeps`), which calls `helm dependency build` and honors the lock. The asymmetry means an unrelated change like adding a `jsonPatches` block to a release can flip a chart from locked-version rendering to "latest matching constraint" rendering, producing surprising image/version drift. Behavior change: - If `Chart.lock` (or legacy `requirements.lock`) exists and no adhoc chart dependencies were injected, run `helm dependency build`. This resolves dependencies from the lock and matches the rest of the helmfile pipeline. - If `dependency build` fails (typically because the lock is out of sync with Chart.yaml), fall back to `dependency up` and log the fallback. This preserves the previous escape hatch for charts whose lock has drifted. - If no lock file exists, or adhoc dependencies are present, behavior is unchanged — still `dependency up`. Adds a unit test for the lock-file detection helper. Signed-off-by: Shane Starcher <shane.starcher@gmail.com>
sstarcher
force-pushed
the
prefer-dep-build-over-up
branch
from
a9c7158 to
d883b8d
Compare
Author
|
This MR paired with this change helmfile/helmfile#2587 fixes my root issue of Chart.locks being ignored and auto updated. |
There was a problem hiding this comment.
Pull request overview
This PR changes chart dependency resolution in Chartify so local charts with lock files prefer reproducible Helm dependency builds over dependency updates.
Changes:
- Adds lock-file detection for
Chart.lockand legacyrequirements.lock. - Switches local-chart dependency resolution to
helm dependency buildwhen a lock exists and no adhoc deps are injected. - Adds unit coverage for the lock-file detection helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
chartify.go |
Updates local chart dependency command selection and adds hasChartLock. |
util_test.go |
Adds tests for hasChartLock lock-file detection. |
| // | ||
| // Adhoc dependencies are appended to Chart.yaml after the lock was generated, so the lock | ||
| // is by definition out of sync — `build` would fail. Fall back to `up` in that case. | ||
| useBuild := len(u.AdhocChartDependencies) == 0 && hasChartLock(tempDir) |
Comment on lines
+407
to
+412
| if err != nil && useBuild { | ||
| // `helm dependency build` errors when Chart.lock is out of sync with Chart.yaml. | ||
| // Fall back to `up` so chartify keeps working on charts whose lock has drifted. | ||
| r.Logf("`helm dependency build` failed for release %s, falling back to `helm dependency up`: %v", release, err) | ||
| depArgs[1] = "up" | ||
| _, err = r.run(nil, r.helmBin(), depArgs...) |
Comment on lines
+407
to
+412
| if err != nil && useBuild { | ||
| // `helm dependency build` errors when Chart.lock is out of sync with Chart.yaml. | ||
| // Fall back to `up` so chartify keeps working on charts whose lock has drifted. | ||
| r.Logf("`helm dependency build` failed for release %s, falling back to `helm dependency up`: %v", release, err) | ||
| depArgs[1] = "up" | ||
| _, err = r.run(nil, r.helmBin(), depArgs...) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When chartify is engaged (e.g. helmfile invokes it for releases that use
jsonPatches, strategic-merge patches, or kustomize), it copies the chart to a temp dir and runshelm dependency upbefore rendering.dep upre-resolves fromChart.yaml's version constraints and rewritesChart.lock, so any constraint that allows newer versions (e.g.version: "*"or~4.1.0) silently pulls a newer dependency on every render.This is asymmetric with the rest of the helmfile pipeline. Helmfile's non-chartify code path (
pkg/helmexec/exec.goBuildDeps) useshelm dependency build, which honors the lock. The result: adding an unrelatedjsonPatchesblock to a release can flip a chart from "locked rendering" to "latest matching constraint rendering," producing surprising image/version drift downstream.We hit this with
aws-efs-csi-driver— the chart hadversion: "*", a committedChart.lockpinning 4.1.0, and akustomization-patch.yaml.helm templaterendered v3.1.0 (the locked appVersion), buthelmfile templaterendered v3.2.0 because chartify randep upand re-resolved to 4.2.0.Change
In
Chartify()'s local-chart branch:Chart.lock(or legacyrequirements.lock) exists and no adhoc deps were injected, runhelm dependency buildinstead ofup.buildfails (typically because the lock is out of sync withChart.yaml), log a fallback and runup. Preserves the existing escape hatch for charts whose lock has drifted.up).Adhoc deps still use
upbecause they're appended toChart.yamlafter the lock was generated;buildwould fail with "lock out of sync."Compatibility
Backward compatible:
up, behavior matches today.Test
Adds
TestHasChartLockcovering presence/absence ofChart.lockandrequirements.lock. Existing integration tests pass (two unrelated pre-existing failures on master —kube_version_and_api_versionssnapshot drift and a kubeconfig-namespace leak — are not affected by this change).