Update helm-diff plugin version from v3.15.11 to v3.15.12 across
Dockerfiles, recommended version constant, CI matrix, and
integration test default.
Signed-off-by: yxxhero <aiopsclub@163.com>
The star history chart in the README is currently broken due to GitHub stargazer API restrictions. Switch the chart to a working alternative so the project's popularity remains visible.
Co-authored-by: Dessalines39394 <245616256+Dessalines39394@users.noreply.github.com>
Bump github.com/helmfile/chartify to v0.28.2, which fixes the empty-render
crash upstream (helmfile/chartify#206, fixed in helmfile/chartify#207):
when a chart renders zero resources, chartify now treats it as a no-op
success itself - removing the chart's content dirs, cleaning Chart.yaml
dependencies and the lock file, and skipping the kustomize step - instead
of failing with:
assertion failed: unexpected dir entry "" it must be the abs path to the output directory
That makes the helmfile-side string-matching workaround from #2724 dead
code: the error it matched can no longer be produced. Remove
isChartifyEmptyRenderOutputError and the special-cased no-op branch in
processChartification, so the empty-render case simply flows through the
regular chartify path.
Also bump github.com/helmfile/vals to v0.46.0 and refresh transitive
dependencies.
The regression test for #1757 is updated to assert the new direct
behavior: processChartification succeeds, returns a chartified chart
whose Chart.yaml no longer declares dependencies (so a subsequent
"helm dep build"/"helm template" cannot fail with "found in Chart.yaml,
but missing in charts/ directory"), renders empty output, and is cleaned
up by CleanupChartifyTempDirs.
Fixes#1757 (follow-up to #2724)
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix(state): prefetch shared remote charts instead of serializing sync/diff (#2741)
PR #2662 fixed a Windows chart-download race (#768) by wrapping the entire
helm upgrade/diff operation in a per-chart+version lock, not just the
download. Since sync/apply/diff never set ForceDownload, releases sharing a
remote chart end up fully serialized even at high --concurrency.
Add ChartPrepareOptions.PrefetchSharedRemoteCharts: PrepareCharts groups
selected releases by chart+version, and force-downloads (once) any chart
used by 2+ releases that also resolve to identical acquisition flags
(--verify/--keyring/--plain-http/--insecure-skip-tls-verify/--devel) and to
a configured repository (or OCI ref) - not a bare \"dir/chart\"-shaped local
path. That materializes release.ChartPath, which lets withChartOperationLock's
existing ChartPath != \"\" guard skip the lock, restoring concurrency without
touching the #768 protection for charts that aren't prefetched.
Also add chartFetchFlags to give forcedDownloadChart's \`helm fetch\` the same
verify/keyring/TLS flags flagsForUpgrade already applies, closing a parity
gap that predates this change (affects lint/unittest/pull too).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Thomas Hanser <gh@toms.place>
* fix(state): exclude verify-enabled charts from shared-chart prefetch, use NUL-delimited flag signature
Copilot review on #2741's PR flagged two issues in the shared-chart prefetch
added there:
1. forcedDownloadChart untars a shared chart into a local directory, but
flagsForUpgrade unconditionally re-adds --verify for the later `helm
upgrade` regardless of ChartPath. Helm's VerifyChart only accepts a
packaged .tgz/provenance pair, not an unpacked directory, so upgrading a
prefetched chart with verify enabled would fail. Exclude --verify from
prefetch eligibility entirely rather than trying to suppress the later
flag - those releases just keep the pre-existing serialized-lock
behavior, unaffected by this feature.
2. The per-key flag-agreement signature joined flags with a space, which
isn't injective: a keyring path containing a space and a flag-like token
could collide with a different keyring plus a real flag, silently
deduplicating releases with different acquisition settings. Join with
NUL instead, which can't appear in an OS argument.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Thomas Hanser <gh@toms.place>
* fix(state): apply -chart override before shared-chart grouping
Copilot flagged that PrepareCharts grouped releases by release.Chart
before prepareChartForRelease applied st.OverrideChart (the -chart CLI
flag), so distinct original charts that all resolve to the same
overridden chart were never recognized as shared and missed the
prefetch. Apply the override once upfront, before the grouping loop
reads release.Chart.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Thomas Hanser <gh@toms.place>
---------
Signed-off-by: Thomas Hanser <gh@toms.place>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: update helm plugins via uninstall+reinstall to honor pinned version (#2726)
helm plugin update re-installs a plugin from its cached source WITHOUT the
--version flag, so it does not reliably install the pinned version helmfile
requests. It reports success (exit 0) while re-downloading the unchanged
cached source, leaving 'helm plugin list' showing the old version.
This affected 'helmfile init --force' which reported a successful diff/secrets
plugin update while the old version remained installed. The reporter's
workaround was to uninstall the plugin before running init.
UpdatePlugin now uninstalls the existing plugin and reinstalls the exact
pinned version via AddPlugin (which passes --version). The unreliable
'plugin update' command is no longer used for the general path; this mirrors
the uninstall+reinstall strategy already used for helm-secrets on Helm 4.
Fixes#2726
Refs #2548
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: only ignore 'not found' uninstall errors in UpdatePlugin
Address review feedback: UpdatePlugin ignored all uninstall errors, which
could mask real failures (permissions, broken Helm) behind a less informative
'plugin already exists' error from the subsequent install.
Now only the expected 'not found' case (plugin removed concurrently, reported
by both Helm 3 'Plugin: <name> not found' and Helm 4 'plugin: <name> not
found') is ignored and install proceeds. Any other uninstall error is returned.
Adds tests for both branches.
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: extract pluginCmd constant to satisfy goconst in tests
golangci-lint (goconst, min-occurrences: 8) flagged the "plugin" string
literal in exec_test.go after the new UpdatePlugin tests pushed the package-
wide count from 7 (under threshold, passing on main) to 14.
Introduce a pluginCmd constant and use it for all helm sub-command checks in
the UpdatePlugin tests. No behavior change; drops the literal count back below
the threshold so CI goconst passes.
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: match helm plugin-absent error precisely; guard against plugin update regression
Address review feedback on UpdatePlugin:
1. The uninstall error check used strings.Contains(err, "not found"), which
is too broad: it also matches unrelated failures such as a missing helm
binary ("executable file not found") or an uninstall hook failing with
"sh: ...: not found". Those would be silently treated as "plugin absent"
and logged/proceeded past, masking the real problem.
Match only helm's specific plugin-absent message instead, via a
case-insensitive regex that covers both majors:
Helm 4: "plugin: <name> not found"
Helm 3: "Plugin: <name> not found"
All other uninstall failures (permissions, missing binary, hook errors)
are now surfaced.
2. The init test mock had no "plugin update" branch, so a production
regression to calling helm plugin update would go undetected (false
negative). Add an explicit case that records and fails on "plugin update".
Also adds a test for the missing-binary case and makes the plugin-absent test
table-driven across Helm 3/4 formats. Extracts installCmd to keep goconst
(min-occurrences: 8) satisfied after the new tests.
Signed-off-by: yxxhero <aiopsclub@163.com>
---------
Signed-off-by: yxxhero <aiopsclub@163.com>
* bump helm-diff recommended version to v3.15.11
* bump helm-diff to v3.15.11 in CI workflow matrix
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* fix: skip chartification when chart renders zero resources
When a chart's templates render zero resources (e.g. everything is
gated behind a falsy `{{- if .Values.enabled }}`) and the release has
transformers, jsonPatches, or strategicMergePatches configured,
helmfile crashed with:
assertion failed: unexpected dir entry "" it must be the abs path
to the output directory
Root cause: chartify's replace.go runs `helm template --output-dir`
and expects exactly one directory entry under that output dir (the
rendered chart). When helm renders no resources, the output dir is
empty, so chartOutputDir stays "" and chartify's own assertion on it
being an absolute path fails. chartify (v0.28.0) doesn't expose a
typed/sentinel error for this, only the assertion text.
Since there's nothing to chartify when a release has no rendered
resources, treat this specific chartify failure as a no-op: keep
using the chart as-is and let helm template it normally (producing
the same empty output helm would have produced without chartify).
Any other chartify error is still surfaced unchanged.
Verified manually end-to-end with a real helm+kustomize:
- a chart with `enabled: false` + a transformer now runs without
error instead of crashing
- a chart with `enabled: true` + the same transformer still gets
transformed correctly (annotations applied), confirming the normal
chartify path is untouched
Added unit tests for the new error-matching helper in
pkg/state/issue_1757_test.go.
Fixes#1757
Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
* fix: return original chart path from empty-render no-op, not the deps-rewrite temp copy
Review feedback on the original fix (#2724) found a real bug: the
empty-render no-op path returned chartPath after it may have already
been reassigned to the temp copy created by rewriteChartDependencies
(for charts with relative file:// deps). That temp dir is removed by
a deferred cleanupTempChart() as soon as processChartification
returns, so the caller was handed a path to a directory that no
longer existed, breaking any subsequent helm command with "chart not
found" - narrow (only local charts with relative file:// deps that
also render zero resources) but real and reproducible.
Capture originalChartPath before the rewrite and return that instead.
Also flatten the nested `if err != nil { if isChartifyEmptyRenderOutputError...`
into two sequential checks per review, and link the upstream tracking
issue (helmfile/chartify#206, opened by a maintainer during review) in
the error-matching constant's doc comment.
Added TestProcessChartification_EmptyRenderReturnsSurvivingPath, an
end-to-end test exercising the real processChartification ->
chartify.Chartify wiring (not just the isChartifyEmptyRenderOutputError
helper) with a chart that has a relative file:// dependency and renders
zero resources - the exact conditions that trigger the bug. Verified
passing against real helm+kustomize in a Linux container; skipped on
Windows due to an unrelated, pre-existing Windows path-handling issue
in chartify's dependency resolution (a drive letter embedded in a
file:// URL gets mis-joined), independent of the code path under test.
Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
* fix: narrow chartify empty-render error match to avoid false positives
Per Copilot's automated review on this PR: the previous substring,
"it must be the abs path to the output directory", matches chartify's
assertion regardless of what chartOutputDir actually is. In the real
empty-render case chartOutputDir is "" (formatted via %q as `""`), but
the same assertion (chartify replace.go:151) would also fire if
chartOutputDir were ever a non-empty-but-still-relative path - a
different, genuine bug that should be surfaced as an error, not
silently treated as an empty-render no-op.
Narrow the match to include the `unexpected dir entry ""` prefix, so
it can only match the exact empty-string case. Verified against the
actual chartify v0.28.0 source (fmt.Errorf with %q on chartOutputDir)
that this is precisely what the empty-render case produces.
Added a test case asserting the same assertion text with a non-empty
dir entry is correctly NOT treated as the empty-render case. Re-ran
the full test suite (including the real helm+kustomize end-to-end
integration test) in a Linux container to confirm the narrower match
still catches the actual reported bug.
Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
---------
Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
* feat: support Helm 4 --rollback-on-failure alongside deprecated --atomic (#2712)
Helm 4 renamed the `--atomic` flag to `--rollback-on-failure`
(helm/helm#13629). The old flag still works under Helm 4 but is
deprecated (prints a warning) and slated for removal in Helm 5.
Add a `rollbackOnFailure` key to both `helmDefaults` (HelmSpec) and
`releases[]` (ReleaseSpec) that emits `--rollback-on-failure`. It
requires Helm 4+ (errors otherwise) and is mutually exclusive with
`atomic`.
Additionally, when the resolved Helm binary is v4+, an existing
`atomic: true` now emits `--rollback-on-failure` instead of `--atomic`,
so users are migrated off the deprecated flag automatically without any
config change. On older Helm, `atomic: true` continues to emit
`--atomic`.
Updated the spew-based values-ID hashes in temp_test.go that change
whenever ReleaseSpec gains a field (same approach as the --force-conflicts
change in #2480).
Closes#2712.
Signed-off-by: yxxhero <aiopsclub@163.com>
* test: add integration test for rollback-on-failure / atomic migration (#2712)
Covers the end-to-end plumbing that unit tests cannot (real helm version
detection + cluster deploy) via test/integration/run.sh:
1. atomic: true parses, deploys a ConfigMap, and emits the version-correct
flag: --rollback-on-failure on Helm 4 (auto-migration of the deprecated
--atomic) and --atomic on Helm 3.
2. rollbackOnFailure: true emits --rollback-on-failure on Helm 4 and is
rejected with a clear Helm-4-required error on Helm 3.
Flag assertions grep the `exec: helm upgrade --install` lines logged under
--debug, matching flags as standalone tokens so the release name
"issue-2712-atomic" cannot be confused with the "--atomic" flag.
Verified locally against Helm 4.2.3: both atomic:true and
rollbackOnFailure:true emit --rollback-on-failure with no --atomic.
Signed-off-by: yxxhero <aiopsclub@163.com>
---------
Signed-off-by: yxxhero <aiopsclub@163.com>
* feat: add --repo-retries for retrying helm repo and registry login commands
Add a configurable retry mechanism for chart repository operations to
handle unstable networks (corporate proxies, slow internal registries).
Closes#1894
- New --repo-retries N flag and HELMFILE_REPO_RETRIES env var (default 0
= opt-in, backward compatible)
- Retry applies to helm repo add, helm repo update (incl. ACR), and
helm registry login with exponential backoff (1s, 2s, 4s, ..., capped 30s)
- Single retryRepoOp helper; per-attempt args/buffer are local to avoid
state leaking across retries
- Tests cover succeed-after-retry, exhausted-retries, disabled-by-default,
and regression guards for password-buffer and args-accumulation
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: address PR review (overflow guard, cancellable sleep, flag-override, docs)
Address Copilot review feedback on #2683:
- Cap backoff shift exponent at 5 to prevent time.Duration overflow on
large --repo-retries values
- Make retry sleep context-aware (sleepCtx) so Ctrl+C aborts the retry
loop promptly via the ShellRunner context
- Log a concise exit status instead of the verbose ExitError dump, and
clarify the retry-counter wording ('retry N/M')
- Use -1 sentinel as the CLI default so --repo-retries=0 can explicitly
disable retries even when HELMFILE_REPO_RETRIES is set
- Align help text and docs: retry applies 'on failure' (not just
transient errors), document the 0-disables behavior
- Add tests for overflow guard, cancellable sleep, and flag-zero-disables
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: abort retries on canceled context, hide sentinel default, align comment
Address follow-up Copilot review on #2683:
- Fix tight-loop bug: sleepCtx now returns whether it completed vs was
interrupted by context cancellation, and retryRepoOp aborts the retry
loop on interruption so Ctrl+C no longer spins into rapid helm calls
- Hide the -1 sentinel from --help by overriding the displayed default
to 0 (pflag DefValue), matching the documented default while keeping
the flag-override semantics
- Correct HelmExecOptions.RepoRetry comment: 'on failure' not 'transient
network errors', matching the actual retry behavior
- Add Test_Retry_AbortsOnCanceledContext covering the no-tight-loop path
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: copy args per retry in RegistryLogin, make cancel test deterministic
Address follow-up Copilot review on #2683:
- RegistryLogin: pass a per-attempt copy of args to execStdIn so its
internal append (for helm.extra) can't alias the shared slice across
retries
- Test_Retry_AbortsOnCanceledContext: cancel the context deterministically
inside the op closure after the first attempt, replacing the flaky
time.Sleep(20ms) goroutine
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: return error on unknown managed repo type instead of silent skip
Address Copilot review on #2683: AddRepo logged an error for an unknown
managed type but returned nil, silently succeeding while skipping the
repo add. Now returns an error so misconfigurations fail loudly.
Signed-off-by: yxxhero <aiopsclub@163.com>
---------
Signed-off-by: yxxhero <aiopsclub@163.com>
dependabot[bot]