The helm.sh/depends-on/resource-groups annotation contains multiple '/'
separators which fails Kubernetes annotation-key validation. The action
layer already strips it before SSA-applying via stripSequencingAnnotations,
but the template path emitted it verbatim, breaking
'helm template --wait=ordered | kubectl apply -f -'.
Lift the annotation list to pkg/release/v1/util.HelmInternalSequencingAnnotations
so both the action layer and the template renderer share one source of truth.
Add StripHelmInternalAnnotations as a line-based regex strip that preserves
surrounding byte order so 'helm template | diff' workflows stay stable.
Apply it in both ordered and flat template render paths and across hook
manifests for symmetry.
Refs HIP-0025.
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Two more findings from Copilot's review of the consolidated commit:
- batchHasCustomReadiness returned true when a single resource had only ONE
of the two readiness annotations. That triggered the custom-readiness status
reader for the whole batch, redundant with warnIfPartialReadinessAnnotations
(which already warns) and with EvaluateCustomReadiness (which falls back to
kstatus when only one is set). Require both annotations to be non-empty
before swapping in the custom reader.
- helm template --wait=ordered hard-failed on YAML parse errors, breaking the
--debug contract that "we always want to print the YAML, even if it is not
valid." Fall back to the flat output path with a stderr warning when ordered
rendering fails, so --debug investigation flows still surface the manifests.
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Two findings from Copilot's review of the consolidated commit on 2026-05-03:
- updateAndWait silently dropped its context arg (named `_`), so sequenced
upgrades and rollbacks never honored cancellation through Build/Update/Wait.
Thread the context through and add ctx.Done() select gates matching
createAndWait.
- GroupManifestsByDirectSubchart used the bare chart name as path prefix.
At top level this is fine because manifest names are rooted there, but on
recursion into a subchart the call passed the subchart's own name (e.g.
"sub") while manifest names are still rooted at the top-level chart
("parent/charts/sub/charts/nested/..."). Result: 3+ deep nesting flattened
into the parent subchart's batch, silently bypassing nested sequencing.
Fix: thread chartPath through deployChartLevel and deleteChartLevelReverse
so each recursion level sees the full path-prefix it should match against.
Add a unit test exercising the deeper-recursion case and update the
uninstall test that had documented this as "a known limitation."
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Implements HIP-0025 to give chart authors first-class control over
deployment ordering of chart resources and subcharts. Helm operators
opt in via --wait=ordered (or WaitStrategy=ordered in the SDK); default
behavior for Chart v2 is unchanged.
== Foundations ==
- DAG abstraction (pkg/chart/v2/util/dag.go) with topological batch
output and cycle detection.
- Resource-group annotation parsing and dependency tracking
(pkg/release/v1/util/resource_group.go) for helm.sh/resource-group
and helm.sh/depends-on/resource-groups; resource IDs are
apiVersion/Kind/Namespace/Name to disambiguate cross-namespace.
- Subchart DAG (pkg/chart/v2/util/subchart_dag.go) reading depends-on
on Chart.yaml dependencies and the helm.sh/depends-on/subcharts
annotation. BuildSubchartDAG inspects c.Dependencies() (post
ProcessDependencies) so it correctly respects conditions, tags, and
aliases - addresses joejulian's review feedback on metadata
heuristics.
- DependsOn []string field on chart.Dependency (pkg/chart/v2/dependency.go).
- SequencingInfo metadata stored on the release object
(pkg/release/v1/release.go) so rollback knows whether a revision
was sequenced.
- Custom readiness via helm.sh/readiness-success and helm.sh/readiness-failure
JSONPath expressions (pkg/kube/readiness.go); falls back to kstatus
if either is missing. Failure conditions take precedence over success.
== Action integration ==
- pkg/action/sequencing.go: sequencedDeployment with per-batch deadline
via min(), context.Done() honored at build/create/wait phases, and
isolated/partial-readiness warnings emitted once per batch (not per
poll tick).
- Install, upgrade, rollback, and uninstall actions consume
WaitStrategy=ordered. Sequenced uninstall and rollback are gated on
the release's stored SequencingInfo to enforce the HIP "reverse
install order" semantic.
- ReadinessTimeout (default 1m) is capped by --timeout and applied
per batch.
- Manifest path recovery for nested subcharts on rollback/uninstall.
== CLI ==
- --wait=ordered on install, upgrade, rollback, AND uninstall. The
AddOrderedWaitFlag helper in pkg/cmd/flags.go is shared across all
four commands.
- --readiness-timeout flag with docstring clarifying that "ready" is
determined by kstatus signals or custom readiness annotations, and
that vanilla Jobs need --wait-for-jobs.
- helm template emits "## START resource-group: <chart> <name>" /
"## END resource-group: ..." delimiters when --wait=ordered. Falls
back to flat manifest output with a warning if YAML parsing fails.
== Lint ==
- pkg/chart/v2/lint/rules/sequencing.go: ErrorSev for circular subchart
deps, partial readiness annotations, and orphan
helm.sh/depends-on/resource-groups references. Empty annotation
values are treated as absent (matches runtime behavior).
== Tests + fixtures ==
- Unit tests for DAG, subchart DAG, lint rules, readiness JSONPath,
resource-group parsing, sequencing action, ordered template output,
and CLI flag wiring. Includes context-cancellation coverage for
sequencedDeployment per joejulian's request.
- Integration testchart at pkg/cmd/testdata/testcharts/sequenced-chart/
exercising parent->subchart and resource-group ordering.
== Backward compatibility ==
Sequencing is gated on WaitStrategy == OrderedWaitStrategy. Charts
without HIP-0025 annotations or --wait=ordered behave exactly as
before. The depends-on field on Chart.yaml dependencies is silently
accepted and unknown to upstream-stable lint (forward-compat fix
to be tracked separately once HIP is accepted).
Refs: HIP-0025
Addresses: joejulian and Copilot review feedback on PR #32038
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Only user-supplied nils should survive coalescing. Chart-default nils
defaults, not just user overrides. This caused:
- %!s(<nil>) in templates using Bitnami common.secrets.key (#31919)
- pluck fallbacks returning nil instead of falling through to globals
(#31971)
Fixes#31919Fixes#31971
Signed-off-by: Johannes Lohmer <jojo.dev@lohmer.com>
Three test cases that cover the regression scenarios introduced
by the #31644 nil preservation fix:
- subchart default nils should be cleaned up
when parent doesn't set those keys (#31919)
- user-supplied null should erase subchart defaults (#31919)
- subchart default nil should not shadow global values via pluck (#31971)
Tests are expected to fail until the regression is fixed.
Signed-off-by: Johannes Lohmer <jojo.dev@lohmer.com>
When running 'helm pull --debug', no debug output was printed because
the HTTP getter did not emit any slog.Debug messages. This adds
slog.Debug calls to log the URL being fetched and the response status
when debug-level logging is enabled.
Fixeshelm/helm#31098
Signed-off-by: Cairon <cairon-ab@users.noreply.github.com>
The previous fix (increasing timeout / reducing deletion delay) did not
work because the flakiness is not a timing problem at all.
Root cause: fluxcd/cli-utils HasSynced() returns true after the initial
list item is *popped* from DeltaFIFO, which is before AddFunc delivers
the ResourceUpdateEvent to the collector. This creates a race where the
SyncEvent can arrive at the statusObserver *before* the pod's Current
status is recorded. When that happens:
- statusObserver sees pod as Unknown
- Unknown is skipped for WaitForDelete (by design, to handle resources
that were already deleted before watching started)
- AggregateStatus([], NotFoundStatus) == NotFoundStatus → cancel()
- The watch context is cancelled before DeleteFunc can fire
- Final check: pod still Current → error
The test intent is to verify that waitForDeleteCtx (not the cancelled
generalCtx) is selected. A non-existent resource satisfies this:
- With waitForDeleteCtx=Background(): informer syncs with empty list
→ Unknown → cancel → success ✓
- With generalCtx (cancelled, wrong): context immediately done
→ ctx.Err() appended → error returned ✓
Remove the goroutine-based deletion and the pod creation to eliminate
the race while preserving the context-selection assertion.
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
TestMethodContextOverridesGeneralContext/WaitForDelete used a 1s
timeout with a 500ms deletion delay, leaving only ~500ms for the
fake watcher to propagate the delete event. On loaded CI runners
this window is too tight and causes intermittent failures.
Increase the timeout to 5s and reduce the deletion delay to 100ms
so there is ample headroom. Apply the same deletion-delay reduction
to TestStatusWaitForDelete which shares the same pattern.
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
When Deployed() succeeds but releaserToV1Release() fails, prepareUpgrade
returned err (nil) instead of cerr (the conversion error), causing a
silent nil return that could lead to nil pointer dereferences downstream.
Closes#32007
Signed-off-by: Rhys McNeill <rhysmcneill7@hotmail.co.uk>
The toTOML doc comment said "returns empty string on marshal error"
but the implementation actually returns err.Error(). Fix the comment
to match the real behavior. Also mention mustToToml as the strict
alternative.
Signed-off-by: Ilya Kiselev <kis-ilya-a@yandex.ru>
Add `mustToToml` that panics on marshal error, consistent with
`mustToYaml` and `mustToJson`. This makes it possible for chart authors
to get a hard failure when TOML serialization fails, rather than having
to inspect the output manually.
`toToml` behavior is unchanged in this commit.
Closes#31430
Signed-off-by: Ilya Kiselev <kis-ilya-a@yandex.ru>
Remove pre-Go modules import path comments from pkg/kube test files
(ready_test.go, resource_test.go, statuswait_test.go) for consistency
with the rest of the package.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
Import path comments (e.g. `// import "helm.sh/helm/v4/pkg/kube"`) are
a pre-Go modules convention no longer needed in module-aware builds.
Some files in pkg/kube had these comments while others did not, causing
inconsistency that triggered downstream Kythe indexing errors.
Remove the import comments from all affected files to make the package
declaration consistent across the directory.
Fixes#31846
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
Rohit Gudi
Terry Howe
Matheus Pimenta
George Jenkins
Evans Mungai
Johannes Lohmer
Johannes Lohmer
Cairon
Anmol Virdi
Rhys McNeill
abhay1999
Debasish Mohanty
Ilya Kiselev
Matthieu MOREL
tsinglua
Travis Leeden