* 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>
* fix: ensure OCI registry login when SkipRepos is set (#1847)
Commands like build, status, list, and show-dag set SkipRepos: true to
avoid slow helm repo add/update for classic repos. However, this also
skipped helm registry login for OCI registries, causing 401 Unauthorized
errors when pulling OCI charts.
Add a variadic SyncOption parameter (backward compatible) with
WithOCIOnly() that limits repo processing to OCI registries only.
When skipRepos is true, callers now pass WithOCIOnly() so that OCI
authentication still happens before chart pulls.
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: reword OCI login comments per review feedback
RegistryLogin is a no-op when credentials are not configured, so the
word 'always' was misleading. Clarify that login is only needed when
credentials are present.
Signed-off-by: yxxhero <aiopsclub@163.com>
* fix: skip OCI login for commands that don't pull charts
Commands like 'list' and 'write-values' skip chart preparation entirely,
so OCI registry login is unnecessary for them. Extract the skip-command
list into a shared variable and use it to gate OCI-only login in
WithPreparedCharts.
Signed-off-by: yxxhero <aiopsclub@163.com>
* docs: clarify commandsSkipChartPrep comment per review feedback
Clarify that these commands only skip OCI login when skipRepos is true;
when skipRepos is false, SyncReposOnce still runs normally for all repos.
Signed-off-by: yxxhero <aiopsclub@163.com>
---------
Signed-off-by: yxxhero <aiopsclub@163.com>
The '========== Updated Releases ==========' header was a fixed 38
chars regardless of the table width below it. Now the divider line
is extended to match the table's visual width, with the title text
centered within the '=' borders.
- Add TableVisualWidth() to measure table width via runewidth
- Add HeaderDividerCentered() and HeaderDividerCenteredStyled()
for centered dividers with optional bold+blue ANSI styling
- Refactor DisplayAffectedReleases to build the table first, then
compute its width before logging the header
- Update all test snapshots and integration test output files
Signed-off-by: yxxhero <aiopsclub@163.com>
