feat: implement HIP-0025 resource creation sequencing#32038
feat: implement HIP-0025 resource creation sequencing#32038
Conversation
pull-request-size
bot
added
the
size/XXL
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements HIP-0025 ordered resource creation sequencing in Helm v4 by introducing DAG-based ordering for subcharts and resource groups, along with optional custom readiness evaluation and CLI support.
Changes:
- Added generic DAG utilities plus subchart/resource-group DAG builders and parsers.
- Implemented
--wait=orderedexecution across install/upgrade/rollback/uninstall and added--readiness-timeout. - Added ordered delimiter output for
helm template, new lint rules, and extensive unit/integration test coverage.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/release/v1/util/resource_group.go | Parses resource-group annotations and builds a DAG for ordered batching. |
| pkg/release/v1/util/resource_group_test.go | Unit tests for resource-group parsing, warnings, and DAG behavior. |
| pkg/release/v1/release.go | Adds SequencingInfo metadata to releases for rollback/uninstall behavior. |
| pkg/release/v1/release_test.go | Tests JSON backward compatibility and round-tripping for SequencingInfo. |
| pkg/kube/readiness.go | Adds custom readiness expression parsing and evaluation via JSONPath. |
| pkg/kube/readiness_test.go | Unit tests for custom readiness evaluation and expression parsing. |
| pkg/kube/custom_readiness_status_reader.go | Integrates custom readiness into kstatus waiting via a StatusReader. |
| pkg/kube/custom_readiness_status_reader_test.go | Verifies waiter integration and status reader behavior. |
| pkg/kube/options.go | Adds waiter option to enable the custom readiness status reader. |
| pkg/kube/client.go | Adds OrderedWaitStrategy and wires in optional custom readiness readers. |
| pkg/kube/client_wait_strategy_test.go | Tests waiter selection and error messaging for ordered strategy. |
| pkg/cmd/flags.go | Adds --wait=ordered parsing support (where applicable) and --readiness-timeout. |
| pkg/cmd/flags_test.go | Tests CLI flag acceptance/rejection and readiness-timeout validation. |
| pkg/cmd/install.go | Wires --readiness-timeout and ordered wait handling into install command. |
| pkg/cmd/upgrade.go | Wires --wait=ordered and --readiness-timeout into upgrade command. |
| pkg/cmd/template.go | Adds ordered template rendering with resource-group delimiters and subchart ordering. |
| pkg/cmd/template_test.go | Adds golden tests for ordered delimiter output and absence without ordered wait. |
| pkg/cmd/testdata/testcharts/sequenced-chart/Chart.yaml | Test fixture chart for ordered template output. |
| pkg/cmd/testdata/testcharts/sequenced-chart/templates/aa-databases-configmap.yaml | Test fixture manifest with resource-group annotation. |
| pkg/cmd/testdata/testcharts/sequenced-chart/templates/bb-app-configmap.yaml | Test fixture manifest with depends-on resource-group annotation. |
| pkg/cmd/testdata/testcharts/sequenced-chart/templates/cc-unsequenced-configmap.yaml | Test fixture manifest without sequencing annotations. |
| pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/Chart.yaml | Test subchart fixture for ordered template output. |
| pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml | Test subchart fixture manifest with resource-group annotation. |
| pkg/cmd/testdata/output/template-ordered-delimiters.txt | Golden output verifying START/END delimiters and ordering. |
| pkg/chart/v2/util/dag.go | Introduces generic DAG with deterministic batching and cycle detection. |
| pkg/chart/v2/util/dag_test.go | Unit tests for DAG batching, cycles, and helper methods. |
| pkg/chart/v2/util/subchart_dag.go | Builds subchart dependency DAG from Chart.yaml fields and annotation. |
| pkg/chart/v2/util/subchart_dag_test.go | Unit tests for subchart DAG ordering, aliases, disabled deps, and errors. |
| pkg/chart/v2/dependency.go | Adds depends-on field to chart dependencies for sequencing declarations. |
| pkg/chart/v2/dependency_json_test.go | Tests JSON/YAML tags and backward compatibility for the new dependency field. |
| pkg/chart/v2/lint/lint.go | Registers new sequencing lint rule. |
| pkg/chart/v2/lint/lint_test.go | Validates sequencing rules are registered and produce expected errors. |
| pkg/chart/v2/lint/rules/sequencing.go | Lint rules for subchart/resource-group cycles and readiness annotation completeness. |
| pkg/chart/v2/lint/rules/sequencing_test.go | Tests sequencing lint behavior across error/warning scenarios. |
| pkg/chart/common/util/jsonschema.go | Minor change to schema validation error formatting implementation. |
| pkg/action/action.go | Refactors render pipeline to optionally return sorted manifests for sequencing-aware actions. |
| pkg/action/sequencing.go | Adds sequenced deployment engine (two-level DAG, per-batch waits, annotation stripping). |
| pkg/action/sequencing_test.go | Comprehensive tests for ordered installs, batching, timeouts, and hooks behavior. |
| pkg/action/install.go | Adds ordered install execution path with per-batch readiness timeout. |
| pkg/action/upgrade.go | Adds ordered upgrade execution path and sequencing metadata recording. |
| pkg/action/upgrade_sequenced_test.go | Tests ordered upgrade behaviors including rollback-on-failure/cleanup and transitions. |
| pkg/action/rollback.go | Adds ordered rollback execution path based on stored sequencing metadata. |
| pkg/action/rollback_sequenced_test.go | Tests ordered rollback behavior, cleanup, and sequencing info preservation. |
| pkg/action/uninstall.go | Adds ordered uninstall path using reverse DAG order when sequencing metadata is present. |
| pkg/action/uninstall_sequenced_test.go | Tests sequenced uninstall reverse ordering, hooks, keep policy/history, and dry-run. |
| pkg/action/warning_system_test.go | Tests slog warnings for partial readiness, isolated groups, and orphan dependencies. |
| pkg/action/backward_compat_test.go | Verifies backward-compat behavior for ordered wait without annotations and legacy releases. |
Comments suppressed due to low confidence (1)
pkg/kube/client.go:1
- Because
newCustomReadinessStatusReader()is prepended and itsSupports()returns true for all kinds, enabling it changes which StatusReader is used for all resources in that wait (including those without custom readiness annotations). If the intent is to only affect resources that actually define both readiness annotations, consider tightening the enablement condition (e.g., only enable the reader when at least one resource in the batch has both annotations) to reduce behavior drift and minimize surprises in mixed batches.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func batchHasCustomReadiness(manifests []releaseutil.Manifest) bool { | ||
| for _, manifest := range manifests { | ||
| if manifest.Head == nil || manifest.Head.Metadata == nil { | ||
| continue | ||
| } | ||
| annotations := manifest.Head.Metadata.Annotations | ||
| if annotations == nil { | ||
| continue | ||
| } | ||
| if annotations[kube.AnnotationReadinessSuccess] != "" || annotations[kube.AnnotationReadinessFailure] != "" { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
This enables the custom readiness status reader for a whole batch even when only one of the readiness annotations is present. That triggers redundant warnings (you already warn in warnIfPartialReadinessAnnotations, and EvaluateCustomReadiness logs too) and needlessly changes the waiter reader stack for resources that will fall back to kstatus anyway. Consider requiring both helm.sh/readiness-success and helm.sh/readiness-failure to be non-empty before returning true, and rely on the existing warning path for partial annotation configs.
There was a problem hiding this comment.
This is intentional since a single annotation triggers a warning via
warnIfPartialReadinessAnnotations, and the reader gracefully falls back to kstatus for resources missing the pair.
Tightening the gate here would suppress that warning path. If a user sets only 1 annotation (readiness-success) they get zero feedback. Linting catches issues pre-deployment, whereas this would be caught during runtime (for charts that don't lint).
caretak3r
force-pushed
the
feature/hip-0025
branch
from
41f231a to
e5409e8
Compare
Document the new resource sequencing feature in Helm v4, including: - Resource group annotations and dependencies - Subchart sequencing via Chart.yaml - Custom readiness conditions with JSONPath - CLI flags (--wait=ordered, --readiness-timeout) - Lint rules for sequencing issues - Execution order for install, upgrade, rollback, and uninstall Refs: HIP-0025, helm/helm#32038 Signed-off-by: promptless[bot] <promptless[bot]@users.noreply.github.com>
|
Promptless prepared a documentation update related to this change. Triggered by helm/helm#32038 Created comprehensive documentation for HIP-0025 resource sequencing, covering the new Review at helm/helm-www#2068 |
caretak3r
force-pushed
the
feature/hip-0025
branch
from
e5409e8 to
da559ec
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkg/kube/readiness.go:1
evaluateExpressionparses JSONPath (jsonpath.New().Parse(...)) on every evaluation. DuringstatusWaiterpolling this can become hot (expressions × resources × poll iterations). To reduce overhead, consider compiling/parsing expressions once (e.g., parse the annotation JSON into a slice of pre-parsed{template, op, expected}structs and reuse compiled JSONPath objects) and reusing across polling iterations for the same resource/batch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| byName := make(map[string]subchartInfo, len(c.Metadata.Dependencies)) | ||
| for _, dep := range c.Metadata.Dependencies { | ||
| name := effectiveDependencyName(dep) | ||
| disabled := !dep.Enabled && dep.Condition == "" && len(dep.Tags) == 0 | ||
| byName[name] = subchartInfo{disabled: disabled} | ||
| if !disabled { | ||
| dag.AddNode(name) | ||
| } |
There was a problem hiding this comment.
The disabled heuristic ignores dependencies that were disabled via condition/tags processing (i.e., dep.Enabled == false but dep.Condition != "" or len(dep.Tags) > 0), which contradicts the function doc (“Disabled subcharts are excluded from the DAG”). A concrete fix is to treat any !dep.Enabled as disabled (and ensure callers run dependency processing so Enabled is meaningful), or alternatively make the contract explicit (e.g., require ProcessDependencies and document it) to avoid misclassifying disabled subcharts.
There was a problem hiding this comment.
The concern here is gating this on !dep.Enabled which would silently produce wrong DAGs when callers haven't processed dependencies.
If I have the callers correct then we have:
| caller | ProcessDependencyEnabled run before? |
|---|---|
| sequencedDeployment | yes - action pipeline processes the deps |
| sequencedUninstall | yes - uses stored release chart |
| helm template --wait=ordered | yes - checkDependencies |
| sequencingRule(lint) | not guaranteed (based on linting rules) |
The Enabled field is only reliable after ProcessDependencyEnabled resolves conditions/tags against values, and BuildSubchartDAG is called from contexts where that processing hasn't necessarily run yet (e.g., lint).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkg/kube/readiness.go:1
- All JSONPath execution errors are currently swallowed and treated as 'condition not met'. This can hide real authoring errors (e.g., incorrect JSONPath that parses but fails at execution time due to type/indexing issues) and lead to resources waiting indefinitely with no actionable signal. Prefer returning an error for execution failures, or at least only suppress a narrow 'field not found / empty result' case while surfacing other failures with context (expression + JSONPath template).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var head releaseutil.SimpleHead | ||
| if err := yaml.Unmarshal([]byte(content), &head); err != nil { | ||
| return nil, fmt.Errorf("YAML parse error on %s: %w", name, err) | ||
| } |
There was a problem hiding this comment.
helm template --wait=ordered now hard-fails if any rendered document cannot be unmarshaled into SimpleHead. This is a behavior change vs the previous template path (which printed manifests without requiring YAML parsing) and can break charts that render non-standard/invalid YAML (especially in scenarios where users rely on --debug to inspect partial output). Consider falling back to the non-ordered/flat output (with a warning) when parsing fails, rather than returning an error.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
pkg/kube/readiness.go:1
- readinessJSONPath()
always prefixes the provided path with.status, which means an expression like{.status.phase} == "Ready"turns into{.status.status.phase}and will never match. This is easy to hit because several fixtures/tests in this PR use{.status.*}style. Consider either (a) explicitly documenting/enforcing that paths are relative to.status(so{.phase}), and updating fixtures accordingly, or (b) supporting both by stripping a leading.status/.status.prefix before prefixing, so{.status.phase}and{.phase}` behave the same.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if orderedTemplateOutput { | ||
| templateChart, err := loadTemplateChart(args, client) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := renderOrderedTemplate(templateChart, strings.TrimSpace(rel.Manifest), out); err != nil { | ||
| return err | ||
| } | ||
| if !client.DisableHooks { |
There was a problem hiding this comment.
The ordered template output path hard-fails if ordered rendering can’t parse/structure the manifests. This conflicts with the surrounding behavior (and comment) that Helm should still print the YAML even when it isn't valid (notably with --debug or partial rendering failures). A more robust approach is to treat ordered rendering as best-effort: if parseTemplateManifests()/renderOrderedTemplate() fails, fall back to printing strings.TrimSpace(rel.Manifest) (and hooks) without delimiters, rather than returning early.
| rawManifests := releaseutil.SplitManifests(targetRelease.Manifest) | ||
| _, sortedManifests, err := releaseutil.SortManifests(rawManifests, nil, releaseutil.InstallOrder) | ||
| if err != nil { | ||
| return fail(nil, fmt.Errorf("parsing target release manifest for sequenced rollback: %w", err)) | ||
| } |
There was a problem hiding this comment.
Sequenced rollback relies on deployChartLevel, which groups manifests by subchart using Manifest.Name path prefixes (e.g., "/charts//..."). The manifests coming from SortManifests are not guaranteed to have those source-path names (often they’re keyed by split keys or resource names), so subchart sequencing can silently degrade into a flat chart-level deployment order. To make rollback sequencing consistent with install/upgrade/uninstall, consider recovering source paths for rollback manifests as well (e.g., re-render via renderResourcesWithFiles using the stored chart+values for that revision, or parse "# Source:" lines and populate Manifest.Name accordingly).
| if err := sd.deployChartLevel(ctx, targetRelease.Chart, sortedManifests); err != nil { | ||
| return fail(sd.createdResources, err) | ||
| } |
There was a problem hiding this comment.
Sequenced rollback relies on deployChartLevel, which groups manifests by subchart using Manifest.Name path prefixes (e.g., "/charts//..."). The manifests coming from SortManifests are not guaranteed to have those source-path names (often they’re keyed by split keys or resource names), so subchart sequencing can silently degrade into a flat chart-level deployment order. To make rollback sequencing consistent with install/upgrade/uninstall, consider recovering source paths for rollback manifests as well (e.g., re-render via renderResourcesWithFiles using the stored chart+values for that revision, or parse "# Source:" lines and populate Manifest.Name accordingly).
| slog.Warn("subchart not found in chart dependencies; deploying without subchart sequencing", | ||
| "subchart", subchartName, | ||
| "batch", batchIdx, | ||
| ) |
There was a problem hiding this comment.
These new warnings use the global slog logger directly, while the rest of the action layer commonly uses cfg.Logger() (which is configurable per Helm invocation). Using the global logger can bypass Helm’s configured log handler/level, making warnings harder to route/disable consistently in different embedding contexts. Prefer emitting warnings via s.cfg.Logger().Warn(...) (or threading a logger into this component) for consistent operational logging behavior.
There was a problem hiding this comment.
Fixed — switching to s.cfg.Logger() for consistent log routing.
| return fmt.Errorf("parsing resource-group annotations: %w", err) | ||
| } | ||
| for _, w := range warnings { | ||
| slog.Warn("resource-group annotation warning", "warning", w) |
There was a problem hiding this comment.
These new warnings use the global slog logger directly, while the rest of the action layer commonly uses cfg.Logger() (which is configurable per Helm invocation). Using the global logger can bypass Helm’s configured log handler/level, making warnings harder to route/disable consistently in different embedding contexts. Prefer emitting warnings via s.cfg.Logger().Warn(...) (or threading a logger into this component) for consistent operational logging behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkg/release/v1/util/resource_group.go:1
- resourceGroupResourceID does not include namespace. If a chart renders same Kind/Name into multiple namespaces (possible via templating), ParseResourceGroups can falsely report “assigned to multiple resource groups” even though the resources are distinct. Include namespace in the identity when available (e.g., apiVersion/kind/namespace/name) to match Kubernetes object identity semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deploy each subchart batch in order | ||
| for batchIdx, batch := range batches { | ||
| for _, subchartName := range batch { | ||
| subManifests := grouped[subchartName] | ||
| if len(subManifests) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| // Find the subchart chart object for recursive nested sequencing | ||
| subChart := findSubchart(chrt, subchartName) | ||
| if subChart == nil { | ||
| // Subchart not found in chart object (may have been disabled or aliased differently) | ||
| // Fall back to flat resource-group deployment for these manifests | ||
| slog.Warn("subchart not found in chart dependencies; deploying without subchart sequencing", | ||
| "subchart", subchartName, | ||
| "batch", batchIdx, | ||
| ) | ||
| if err := s.deployResourceGroupBatches(ctx, subManifests); err != nil { | ||
| return fmt.Errorf("deploying subchart %s resources: %w", subchartName, err) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Recursively deploy the subchart (handles its own nested subcharts and resource-groups) | ||
| if err := s.deployChartLevel(ctx, subChart, subManifests); err != nil { | ||
| return fmt.Errorf("deploying subchart %s: %w", subchartName, err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description states “parallel within batch” for subchart batches, but the implementation processes each subchart in a batch sequentially. If parallelism is intended, consider executing each subchart in a batch concurrently (and joining errors) before proceeding to the next batch. Tradeoff: concurrency will improve rollout time for independent subcharts but requires careful aggregation of per-subchart errors and avoiding interleaved logging/output confusion.
There was a problem hiding this comment.
Sequential within a batch is “intentional" for this PR. The parallel execution adds goroutine fan-out and error aggregation complexity. Intra-batch parallelism can be a follow up improvement once the ordering semantics are stable.
| chartsPrefix := chartManifestPrefix(chartPath) + "/charts/" | ||
|
|
||
| for _, manifest := range manifests { | ||
| if !strings.HasPrefix(manifest.Name, chartsPrefix) { | ||
| result[""] = append(result[""], manifest) | ||
| continue | ||
| } | ||
|
|
||
| rest := manifest.Name[len(chartsPrefix):] | ||
| idx := strings.Index(rest, "/") | ||
| if idx < 0 { | ||
| result[""] = append(result[""], manifest) | ||
| continue | ||
| } | ||
|
|
||
| subchartName := rest[:idx] | ||
| result[subchartName] = append(result[subchartName], manifest) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
There was a problem hiding this comment.
There are now two different implementations of “group manifests by subchart” logic: GroupManifestsByDirectSubchart (action) and groupManifestsByChartPath/chartManifestPrefix (cmd/template). This duplication is likely to drift (especially around nested subcharts and alias handling). Consider extracting a shared helper (e.g., in pkg/release/v1/util or pkg/chart/v2/util) that works off a chart-path concept and can be reused by action, uninstall logic, and template rendering.
| chartsPrefix := chartManifestPrefix(chartPath) + "/charts/" | |
| for _, manifest := range manifests { | |
| if !strings.HasPrefix(manifest.Name, chartsPrefix) { | |
| result[""] = append(result[""], manifest) | |
| continue | |
| } | |
| rest := manifest.Name[len(chartsPrefix):] | |
| idx := strings.Index(rest, "/") | |
| if idx < 0 { | |
| result[""] = append(result[""], manifest) | |
| continue | |
| } | |
| subchartName := rest[:idx] | |
| result[subchartName] = append(result[subchartName], manifest) | |
| } | |
| return result | |
| } | |
| for _, manifest := range manifests { | |
| subchartName, ok := directSubchartForManifest(chartPath, manifest.Name) | |
| if !ok { | |
| result[""] = append(result[""], manifest) | |
| continue | |
| } | |
| result[subchartName] = append(result[subchartName], manifest) | |
| } | |
| return result | |
| } | |
| func directSubchartForManifest(chartPath, manifestName string) (string, bool) { | |
| chartsPrefix := chartManifestPrefix(chartPath) + "/charts/" | |
| rest, ok := strings.CutPrefix(manifestName, chartsPrefix) | |
| if !ok { | |
| return "", false | |
| } | |
| subchartName, _, ok := strings.Cut(rest, "/") | |
| if !ok || subchartName == "" { | |
| return "", false | |
| } | |
| return subchartName, true | |
| } |
There was a problem hiding this comment.
Acknowledged — there's drift risk here. Deferring the shared helper extraction to a follow-up to keep this PR scoped; will file a tracking issue.
| for _, batch := range batches { | ||
| for _, groupName := range batch { | ||
| fmt.Fprintf(out, "## START resource-group: %s %s\n", chartPath, groupName) | ||
| for _, manifest := range result.Groups[groupName] { | ||
| fmt.Fprintf(out, "---\n%s\n", manifest.Content) | ||
| } | ||
| fmt.Fprintf(out, "## END resource-group: %s %s\n", chartPath, groupName) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, manifest := range result.Unsequenced { | ||
| fmt.Fprintf(out, "---\n%s\n", manifest.Content) | ||
| } |
There was a problem hiding this comment.
In ordered template mode, the output includes Helm-internal sequencing annotations (e.g., "helm.sh/depends-on/resource-groups") that are not valid Kubernetes annotation keys due to multiple slashes, and Helm only strips them during apply/upgrade flows. This makes helm template --wait=ordered | kubectl apply -f - fail. Consider emitting a warning on stderr when ordered delimiters are enabled (or documenting clearly in the --wait=ordered/template help text) that ordered template output is not meant to be applied directly without stripping these internal annotations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func resourceGroupResourceID(manifest Manifest) string { | ||
| if manifest.Head == nil || manifest.Head.Metadata == nil || manifest.Head.Metadata.Name == "" { | ||
| return manifest.Name | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s/%s/%s", manifest.Head.Version, manifest.Head.Kind, manifest.Head.Metadata.Name) |
There was a problem hiding this comment.
resourceGroupResourceID uses only apiVersion/kind/name to detect duplicate resource assignments. This can incorrectly treat two same-named resources in different namespaces as the same object, potentially raising a false "assigned to multiple resource groups" error. Consider including metadata.namespace in the ID when present (e.g., extend SimpleHead metadata to capture Namespace) so namespaced resources are uniquely identified.
| func readinessJSONPath(path string) (string, error) { | ||
| switch { | ||
| case strings.HasPrefix(path, "."): | ||
| return "{.status" + path + "}", nil | ||
| case strings.HasPrefix(path, "["): | ||
| return "{.status" + path + "}", nil | ||
| default: | ||
| return "", fmt.Errorf("invalid JSONPath %q: path must start with . or [", path) | ||
| } |
There was a problem hiding this comment.
readinessJSONPath unconditionally prepends ".status" to the user-provided path. If a chart author writes expressions like {.status.phase} == "Ready" (which is a common/expected form and is used in this PR's lint tests), the effective JSONPath becomes .status.status.phase and will never match. Either update examples/tests/docs to require paths relative to status (e.g., {.phase}), or make readinessJSONPath detect an explicit .status prefix and avoid double-wrapping.
| if !hasSuccess || !hasFailure { | ||
| if hasSuccess || hasFailure { | ||
| slog.Warn( | ||
| "only one custom readiness annotation present; falling back to kstatus", | ||
| "resource", obj.GetName(), | ||
| "hasReadinessSuccess", hasSuccess, | ||
| "hasReadinessFailure", hasFailure, | ||
| ) | ||
| } | ||
| return ReadinessPending, true, nil | ||
| } |
There was a problem hiding this comment.
EvaluateCustomReadiness logs a warning whenever only one of the custom readiness annotations is present. Since this function is called repeatedly by the kstatus polling engine, a single misconfigured resource can emit the same warning many times (log spam) during a wait. Consider emitting this warning once per resource (e.g., in the status reader with a cache keyed by UID/GVK+ns/name), or moving the warning responsibility to the action/lint layers and keeping the evaluator side-effect-free.
| for _, batch := range batches { | ||
| for _, groupName := range batch { | ||
| fmt.Fprintf(out, "## START resource-group: %s %s\n", chartPath, groupName) | ||
| for _, manifest := range result.Groups[groupName] { | ||
| fmt.Fprintf(out, "---\n%s\n", manifest.Content) | ||
| } |
There was a problem hiding this comment.
The rendered manifests printed by helm template will include helm.sh/depends-on/resource-groups, but this annotation key format is not a valid Kubernetes qualified name (it contains multiple '/'), which prevents users from piping helm template | kubectl apply. Since Helm already strips this annotation before apply in the action layer, consider also stripping Helm-internal sequencing annotations from template output (ordered and non-ordered) to keep the output directly apply-able.
There was a problem hiding this comment.
Renaming to helm.sh/resource-group-depends-on makes this valid, but breaks the HIP-0025 standards for the annotations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| f.IntVar(&client.MaxHistory, "history-max", settings.MaxHistory, "limit the maximum number of revisions saved per release. Use 0 for no limit") | ||
| addDryRunFlag(cmd) | ||
| AddWaitFlag(cmd, &client.WaitStrategy) | ||
| addReadinessTimeoutFlag(f, &client.ReadinessTimeout) |
There was a problem hiding this comment.
rollback now registers the --readiness-timeout flag, but --readiness-timeout is documented/validated for install and upgrade only, and the updated flags tests expect rollback to not have this flag. Keeping it here will fail TestReadinessTimeout and exposes an option that rollback doesn’t currently apply in a user-visible way. Remove the addReadinessTimeoutFlag call from rollback (or, if rollback should support it, update the CLI docs/tests and wire it into rollback execution consistently).
| addReadinessTimeoutFlag(f, &client.ReadinessTimeout) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Version int | ||
| Timeout time.Duration | ||
| ReadinessTimeout time.Duration | ||
| WaitStrategy kube.WaitStrategy |
There was a problem hiding this comment.
ReadinessTimeout is added to Rollback and used for sequenced rollbacks, but rollback doesn’t enforce the same constraint as install/upgrade (ReadinessTimeout must not exceed Timeout). Add a validation (e.g., early in Run) to prevent misconfiguration for action-package callers.
caretak3r
force-pushed
the
feature/hip-0025
branch
from
e3c0e2a to
0c95ee2
Compare
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
Includes the readiness rollback flag fix for the integration readiness script. Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
correct thing to do here is return `cerr` otherwise decode failure silently vanish Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: rohit <50377477+caretak3r@users.noreply.github.com>
avoid mis-matching charts nested in deeper paths Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: rohit <50377477+caretak3r@users.noreply.github.com>
Reverting original change, in favor or non-standard YAML conventions. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: rohit <50377477+caretak3r@users.noreply.github.com>
…very, slog import
- Replace global slog.Warn calls with cfg.Logger() in sequencing.go
- Deduplicate partial-readiness warning (fire once per batch, not per poll tick)
- Fix readinessJSONPath double-prefix when user writes {.status.phase}
- Add Namespace to SimpleHead.Metadata and resourceGroupResourceID
- Align lint annotation check with runtime (TrimSpace for empty values)
- Fix JSONPath test fixtures from {.status.phase} to {.phase}
- Update flags_test to expect --readiness-timeout on rollback
- Fix warning_system_test logger capture to use cfg.SetLogger()
- Extract renderReleaseManifestsWithPaths as shared function
caretak3r
force-pushed
the
feature/hip-0025
branch
from
0c95ee2 to
98a05bb
Compare
joejulian
left a comment
joejulian
left a comment
There was a problem hiding this comment.
I've offered a couple changes, one of which will need duplicated throughout the sequencing.go file, but you can identify where using the provided test, and a broader change to subchart_dag.go
| if s.upgradeMode { | ||
| return s.updateAndWait(ctx, manifests) | ||
| } | ||
|
|
There was a problem hiding this comment.
| if s.upgradeMode { | |
| return s.updateAndWait(ctx, manifests) | |
| } | |
| if s.upgradeMode { | |
| return s.updateAndWait(ctx, manifests) | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| } |
|
|
||
| assert.Contains(t, buf.String(), "readiness") | ||
| } | ||
|
|
There was a problem hiding this comment.
| func TestSequencedDeployment_CreateAndWait_RespectsContextCancellation(t *testing.T) { | |
| newSequencedDeploymentForTest := func(client kube.Interface) *sequencedDeployment { | |
| t.Helper() | |
| cfg := actionConfigFixture(t) | |
| cfg.KubeClient = client | |
| return &sequencedDeployment{ | |
| cfg: cfg, | |
| releaseName: "demo", | |
| releaseNamespace: "spaced", | |
| waitStrategy: kube.OrderedWaitStrategy, | |
| readinessTimeout: time.Minute, | |
| } | |
| } | |
| t.Run("context canceled before build", func(t *testing.T) { | |
| client := newRecordingKubeClient() | |
| sd := newSequencedDeploymentForTest(client) | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| cancel() | |
| err := sd.createAndWait(ctx, []releaseutil.Manifest{ | |
| makeTestManifest("cm", "chart/templates/cm.yaml", nil), | |
| }) | |
| require.ErrorIs(t, err, context.Canceled) | |
| assert.Empty(t, client.createCalls) | |
| assert.Empty(t, client.waitCalls) | |
| }) | |
| t.Run("context canceled after build before create", func(t *testing.T) { | |
| client := newRecordingKubeClient() | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| client.onBuild = cancel | |
| sd := newSequencedDeploymentForTest(client) | |
| err := sd.createAndWait(ctx, []releaseutil.Manifest{ | |
| makeTestManifest("cm", "chart/templates/cm.yaml", nil), | |
| }) | |
| require.ErrorIs(t, err, context.Canceled) | |
| assert.Empty(t, client.createCalls) | |
| assert.Empty(t, client.waitCalls) | |
| }) | |
| t.Run("context canceled after create before wait", func(t *testing.T) { | |
| client := newRecordingKubeClient() | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| client.onCreate = cancel | |
| sd := newSequencedDeploymentForTest(client) | |
| err := sd.createAndWait(ctx, []releaseutil.Manifest{ | |
| makeTestManifest("cm", "chart/templates/cm.yaml", nil), | |
| }) | |
| require.ErrorIs(t, err, context.Canceled) | |
| require.Len(t, client.createCalls, 1) | |
| assert.Empty(t, client.waitCalls) | |
| }) | |
| } | |
| @@ -0,0 +1,127 @@ | |||
| /* | |||
There was a problem hiding this comment.
This re-derives dependency enablement from metadata fields that are no longer the source of truth by this point. Helm already resolves dependency enablement in ProcessDependencies so when BuildSubchartDAG runs, the meaningful question is not "does this dependency stuct look disabled according to heuristic," it's "did this subchart survive dependency processing and remain in c.Dependencies()?"
This is a patch that changes the DAG builder to use that post-processed chart state directly. It matters for 3 reasons:
- It removes duplicate logic
- It makes sequencing match what Helm will actually render and install
- It avoids misclassification around conditions, tags, and aliases.
diff --git a/pkg/chart/v2/util/subchart_dag.go b/pkg/chart/v2/util/subchart_dag.go
index <old>..<new> 100644
--- a/pkg/chart/v2/util/subchart_dag.go
+++ b/pkg/chart/v2/util/subchart_dag.go
@@ -16,11 +16,7 @@ limitations under the License.
package util
import (
"encoding/json"
"fmt"
- "log/slog"
chart "helm.sh/helm/v4/pkg/chart/v2"
)
@@ -31,10 +27,6 @@ const (
AnnotationDependsOnSubcharts = "helm.sh/depends-on/subcharts"
)
-type subchartInfo struct {
- disabled bool
-}
-
// BuildSubchartDAG constructs a DAG from a chart's subchart dependency declarations.
//
// Ordering is read from:
@@ -50,18 +42,28 @@ func BuildSubchartDAG(c *chart.Chart) (*DAG, error) {
return dag, nil
}
- byName := make(map[string]subchartInfo, len(c.Metadata.Dependencies))
+ loaded := make(map[string]struct{}, len(c.Dependencies()))
+ for _, depChart := range c.Dependencies() {
+ loaded[depChart.Name()] = struct{}{}
+ }
+
+ byName := make(map[string]bool, len(c.Metadata.Dependencies))
for _, dep := range c.Metadata.Dependencies {
+ if dep == nil {
+ continue
+ }
+
name := effectiveDependencyName(dep)
- disabled := !dep.Enabled && dep.Condition == "" && len(dep.Tags) == 0
- byName[name] = subchartInfo{disabled: disabled}
- if !disabled {
+ _, enabled := loaded[name]
+ byName[name] = enabled
+ if enabled {
dag.AddNode(name)
}
}
for _, dep := range c.Metadata.Dependencies {
+ if dep == nil {
+ continue
+ }
name := effectiveDependencyName(dep)
- if byName[name].disabled {
+ if !byName[name] {
continue
}
@@ -84,35 +86,27 @@ func BuildSubchartDAG(c *chart.Chart) (*DAG, error) {
}
for subchartName, prerequisites := range declared {
- info, ok := byName[subchartName]
- if !ok {
- return nil, fmt.Errorf("annotation %s references unknown subchart %q", AnnotationDependsOnSubcharts, subchartName)
- }
- if info.disabled {
- slog.Info("skipping annotation dependency for disabled subchart", "subchart", subchartName)
- continue
+ enabled, ok := byName[subchartName]
+ if !ok || !enabled {
+ return nil, fmt.Errorf("annotation %s references unknown or disabled subchart %q", AnnotationDependsOnSubcharts, subchartName)
}
for _, prerequisite := range prerequisites {
if err := addSubchartEdge(dag, byName, subchartName, prerequisite); err != nil {
return nil, err
@@ -122,18 +116,13 @@ func BuildSubchartDAG(c *chart.Chart) (*DAG, error) {
return dag, nil
}
-func addSubchartEdge(dag *DAG, byName map[string]subchartInfo, subchartName, prerequisite string) error {
- info, ok := byName[prerequisite]
- if !ok {
- return fmt.Errorf("subchart %q depends-on unknown subchart %q", subchartName, prerequisite)
- }
- if info.disabled {
- slog.Info("ignoring dependency on disabled subchart", "subchart", subchartName, "disabledPrerequisite", prerequisite)
- return nil
+func addSubchartEdge(dag *DAG, byName map[string]bool, subchartName, prerequisite string) error {
+ enabled, ok := byName[prerequisite]
+ if !ok || !enabled {
+ return fmt.Errorf("subchart %q depends-on unknown or disabled subchart %q", subchartName, prerequisite)
}
if err := dag.AddEdge(prerequisite, subchartName); err != nil {
return fmt.Errorf("adding sequencing edge %s→%s: %w", prerequisite, subchartName, err)
}
Thank you very much for your comment. Will come back to this tomorrow! |
What this PR does / why we need it
This PR implements HIP-0025: Better Support for Resource Creation Sequencing, adding native DAG-based ordering for resource groups and subcharts to Helm v4.
Today, Helm applies all rendered manifests to the cluster simultaneously. Application distributors who need ordered deployment must resort to hooks (tedious to maintain) or bake sequencing into the application itself (unnecessary complexity). HIP-0025 gives chart authors a first-class mechanism to define deployment order using annotations and
Chart.yamlfields, and gives operators a--wait=orderedflag to enable sequenced execution.Overview
When
--wait=orderedis specified, Helm builds two levels of dependency graphs:depends-onfields inChart.yamldependencies and/or thehelm.sh/depends-on/subchartsannotation.helm.sh/resource-groupandhelm.sh/depends-on/resource-groupsannotations.Resources are deployed in topological order (Kahn's algorithm). Helm waits for each batch to reach readiness before proceeding to the next. Readiness defaults to kstatus evaluation, but chart authors can override it with custom JSONPath expressions via
helm.sh/readiness-successandhelm.sh/readiness-failureannotations supporting 6 operators (==,!=,<,<=,>,>=).Execution Flow
Commit Organization
This PR is organized into 7 logical commits to aid review. Each builds on the previous:
feat(hip-0025): add sequencing DAG foundationsAddNode,AddEdge,GetBatches, cycle detection, deterministic batching (pkg/chart/v2/util/dag.go)feat(hip-0025): add resource group parsing and readinesspkg/release/v1/util/resource_group.go), subchart DAG builder fromChart.yamlDependsOn+ annotation (pkg/chart/v2/util/subchart_dag.go), custom readiness eval via JSONPath (pkg/kube/readiness.go)feat(hip-0025): add sequencing schema metadataSequencingInfoonRelease,OrderedWaitStrategyconstant,DependsOn []stringonchart.Dependencyfeat(hip-0025): sequence install upgrade rollback and uninstallpkg/action/sequencing.go, modifications toinstall.go,upgrade.go,rollback.go,uninstall.go)feat(hip-0025): add ordered wait flags and template output--wait=orderedand--readiness-timeoutCLI flags,helm templateresource-group delimiters (## START/END resource-group:)feat(hip-0025): add sequencing lint rules and warningstest(hip-0025): add sequencing tests and integration fixturesNew Annotations
helm.sh/resource-grouphelm.sh/depends-on/resource-groupshelm.sh/depends-on/subchartshelm.sh/readiness-successhelm.sh/readiness-failureNew Chart.yaml Fields
depends-onondependencies[]entries — declares subchart ordering dependencies by name or aliasNew CLI Flags
--wait=orderedinstall,upgrade--readiness-timeoutinstall,upgrade--timeout)Schema Changes
DependsOn []stringadded tochart.DependencySequencingInfostruct added torelease.Release(Enabled bool,Strategy string)OrderedWaitStrategyconstant addedKey Design Decisions
--wait=orderedare upgraded/rolled back/uninstalled using the traditional single-batch flow.helm.sh/hook-weightas before.How to Test
Unit tests:
Integration tests (requires kind cluster):
Manual verification checklist:
helm templatewith--wait=orderedshows resource-group delimitershelm lintdetects circular dependencies, partial readiness, and orphan refs--readiness-timeoutis honored per resource groupSequencingInfowhen--wait=orderedis usedReference Documents
Special notes for reviewers:
make test-unit,make lint,make vet). There is a known pre-existing DATA RACE inpkg/repo/v1.TestConcurrencyDownloadIndexthat is not related to this PR.If applicable:
Signed-off-by: Rohit Gudi 50377477+caretak3r@users.noreply.github.com
refs https://github.com/helm/community/blob/main/hips/hip-0025.md