feat: implement native Helm release lifecycle controls - #2847
feat: implement native Helm release lifecycle controls#2847Mikhail Shirkov (shirkevich) wants to merge 28 commits into
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
GitHub Actions
Inactive
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Mikhail Shirkov (shirkevich)
changed the title
Mikhail Shirkov (shirkevich)
added
the
minor
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (34)
🚧 Files skipped from review as they are similar to previous changes (28)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughHelm now supports release lifecycle policies across schemas, stack inheritance, CLI flags, validation, and install, upgrade, and uninstall execution. Dry-run handling, lifecycle summaries, namespace handling, external-target validation, and related tests were added. ChangesHelm lifecycle support
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The lifecycle conversion correctly rejects invalid values, and no merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant HelmCommand
participant StackProcessor
participant HelmExecutor
participant HelmClient
participant Kubernetes
HelmCommand->>StackProcessor: lifecycle configuration and overrides
StackProcessor->>HelmExecutor: merged Helm settings
HelmExecutor->>HelmClient: chartSpec and dry-run state
HelmClient->>Kubernetes: install, upgrade, or uninstall release
Kubernetes-->>HelmClient: releaseActionResult
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.98% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 127 functions across 32 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/datafetcher/schema/stacks/stack-config/1.0.json (1)
855-862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
wait_strategyenum definition.This schema repeats the
wait_strategyenum (watcher,hookOnly,legacy) in the "helm" defaults block and in "helm_component_manifest".pkg/datafetcher/schema/atmos/manifest/1.0.jsonavoids this by defininghelm_wait_strategyonce and referencing it with$refin both places. Apply the same pattern here to keep the two copies from diverging.♻️ Proposed refactor
"definitions": { + "helm_wait_strategy": { + "title": "helm_wait_strategy", + "description": "Helm 4 release wait strategy", + "type": "string", + "enum": ["watcher", "hookOnly", "legacy"] + }, "helm_repository": {Then, in both the "helm" and "helm_component_manifest" definitions:
- "wait_strategy": { - "type": "string", - "enum": [ - "watcher", - "hookOnly", - "legacy" - ] - }, + "wait_strategy": { + "$ref": "`#/definitions/helm_wait_strategy`" + },Also applies to: 998-1005
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/datafetcher/schema/stacks/stack-config/1.0.json` around lines 855 - 862, Define a shared `helm_wait_strategy` enum schema containing `watcher`, `hookOnly`, and `legacy`, then replace the duplicated `wait_strategy` enum definitions in both the `helm` defaults block and `helm_component_manifest` with `$ref` references to it, matching the pattern used in the atmos manifest schema.cmd/helm/helm.go (1)
165-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep Helm history defaults in one place.
cmd/helm/helm.gousesdefaultHelmHistoryMax = 10, whilepkg/component/helm/lifecycle.gouses a separatedefaultHelmMaxHistory = 10. Use a shared constant for the CLI flag and lifecycle default so future changes do not drift between help text,--history-maxbehavior, and the lifecycle resolver’s applied policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/helm/helm.go` at line 165, Consolidate the duplicate Helm history defaults by defining one shared constant and using it for both the cmd/helm/helm.go history-max flag and the pkg/component/helm/lifecycle.go defaultHelmMaxHistory resolver behavior. Remove the separate defaultHelmHistoryMax and defaultHelmMaxHistory definitions while preserving the current value and CLI/help behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/component/helm/client_test.go`:
- Around line 59-97: Upgrade the pinned helm.sh/helm/v4 dependency to a release
containing the RollbackOnFailure WaitStrategy fix, ensuring
configureInstallLifecycle and configureUpgradeLifecycle no longer allow the
internal rollback/uninstall path to fail with “unknown wait strategy.”
In `@pkg/component/helm/executor.go`:
- Around line 164-172: Update the lifecycle warning loop in the apply/delete
operation path to send each warning through the command UI’s stderr/status
output mechanism instead of log.Warn. Preserve the warning message and its
field/code context using the established ui.Write* or equivalent UI symbol
available in the surrounding executor flow.
---
Nitpick comments:
In `@cmd/helm/helm.go`:
- Line 165: Consolidate the duplicate Helm history defaults by defining one
shared constant and using it for both the cmd/helm/helm.go history-max flag and
the pkg/component/helm/lifecycle.go defaultHelmMaxHistory resolver behavior.
Remove the separate defaultHelmHistoryMax and defaultHelmMaxHistory definitions
while preserving the current value and CLI/help behavior.
In `@pkg/datafetcher/schema/stacks/stack-config/1.0.json`:
- Around line 855-862: Define a shared `helm_wait_strategy` enum schema
containing `watcher`, `hookOnly`, and `legacy`, then replace the duplicated
`wait_strategy` enum definitions in both the `helm` defaults block and
`helm_component_manifest` with `$ref` references to it, matching the pattern
used in the atmos manifest schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dcfc3bd-8fdb-4470-b0cc-9df9fd9be979
📒 Files selected for processing (27)
cmd/helm/helm.gocmd/helm/helm_test.goerrors/errors.gointernal/exec/stack_processor_merge.gointernal/exec/stack_processor_merge_test.gointernal/exec/stack_processor_process_stacks.gointernal/exec/stack_processor_process_stacks_helpers.gointernal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/stack_processor_process_stacks_helpers_test.gointernal/exec/stack_processor_process_stacks_test.gopkg/component/helm/chart.gopkg/component/helm/client.gopkg/component/helm/client_lifecycle_test.gopkg/component/helm/client_test.gopkg/component/helm/executor.gopkg/component/helm/executor_extra_test.gopkg/component/helm/executor_test.gopkg/component/helm/lifecycle.gopkg/component/helm/lifecycle_test.gopkg/component/helm/provision.gopkg/component/helm/provision_test.gopkg/component/helm/values.gopkg/config/const.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/datafetcher/schema_helm_lifecycle_validation_test.gopkg/datafetcher/schema_section_coverage_test.go
fc4bb7e to
3fbac27
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 34 minutes. |
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
GitHub Actions
Inactive
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 48 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/component/helm/lifecycle.go (1)
97-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a warning for the silent rollback-driven wait strategy override.
validateAndDeriveLifecyclechangesWaitStrategyfromHookOnlyStrategytoStatusWatcherStrategywheneverRollbackOnFailureis true. This change has no observable trace: nolifecycleWarningis appended, unlikewarningAtomicDeprecated,warningWaitIgnored,warningWaitBoolean, andwarningTimeoutMigration.A user who sets
wait_strategy: hookOnlyexplicitly, together withrollback_on_failure: true, gets a different wait strategy than requested, with no signal inresolution.Warningsto explain why. Add a warning code for this derivation so the change is visible in lifecycle summaries and logs.♻️ Proposed fix to surface the derived override
+const warningWaitStrategyDerived lifecycleWarningCode = "wait_strategy_derived_from_rollback" + func validateAndDeriveLifecycle(resolution *releaseLifecycleResolution) error { if resolution.Policy.RollbackOnFailure && resolution.Policy.WaitStrategy == kube.HookOnlyStrategy { resolution.Policy.WaitStrategy = kube.StatusWatcherStrategy + resolution.Warnings = append(resolution.Warnings, lifecycleWarning{ + Code: warningWaitStrategyDerived, + Field: cfg.HelmWaitStrategySectionName, + Message: "helm wait_strategy was changed to 'watcher' because rollback_on_failure requires monitoring release status", + }) } if resolution.Policy.WaitForJobs && resolution.Policy.WaitStrategy == kube.HookOnlyStrategy { return errUtils.ErrHelmWaitForJobsRequiresWait } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/component/helm/lifecycle.go` around lines 97 - 105, Add a dedicated lifecycle warning code for the rollback-driven wait-strategy override, following the existing warning symbols such as warningAtomicDeprecated and warningTimeoutMigration. In validateAndDeriveLifecycle, append that warning to resolution.Warnings whenever RollbackOnFailure changes HookOnlyStrategy to StatusWatcherStrategy, while preserving the existing wait-for-jobs validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/helm/helm.go`:
- Around line 157-175: Update the duplicated --wait help descriptions in
addLifecycleOperationFlags for both apply and delete to document the accepted
legacy boolean values true and false alongside watcher, hookOnly, and legacy;
preserve the existing resolver behavior and default watcher selection.
In `@pkg/component/helm/client.go`:
- Around line 74-75: Update the Helm action failure returns in the history
inspection branch and the corresponding lines around the later Helm action
handling to wrap distinct static sentinel errors from errors/errors.go, while
preserving the Helm error as the wrapped cause and existing contextual messages.
Reuse an existing sentinel where appropriate or add clearly named static errors,
ensuring callers can use errors.Is() to identify each failure category.
---
Nitpick comments:
In `@pkg/component/helm/lifecycle.go`:
- Around line 97-105: Add a dedicated lifecycle warning code for the
rollback-driven wait-strategy override, following the existing warning symbols
such as warningAtomicDeprecated and warningTimeoutMigration. In
validateAndDeriveLifecycle, append that warning to resolution.Warnings whenever
RollbackOnFailure changes HookOnlyStrategy to StatusWatcherStrategy, while
preserving the existing wait-for-jobs validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3769a387-c520-4d4f-bf22-166e16bf3f3f
📒 Files selected for processing (27)
cmd/helm/helm.gocmd/helm/helm_test.goerrors/errors.gointernal/exec/stack_processor_merge.gointernal/exec/stack_processor_merge_test.gointernal/exec/stack_processor_process_stacks.gointernal/exec/stack_processor_process_stacks_helpers.gointernal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/stack_processor_process_stacks_helpers_test.gointernal/exec/stack_processor_process_stacks_test.gopkg/component/helm/chart.gopkg/component/helm/client.gopkg/component/helm/client_lifecycle_test.gopkg/component/helm/client_test.gopkg/component/helm/executor.gopkg/component/helm/executor_extra_test.gopkg/component/helm/executor_test.gopkg/component/helm/lifecycle.gopkg/component/helm/lifecycle_test.gopkg/component/helm/provision.gopkg/component/helm/provision_test.gopkg/component/helm/values.gopkg/config/const.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/datafetcher/schema_helm_lifecycle_validation_test.gopkg/datafetcher/schema_section_coverage_test.go
GitHub Actions
Inactive
TestDecodeReleasePolicyValidation only exercised the enum/unknown-field failures, leaving the wrong-type branches in decodeReleasePolicy and its per-operation decoders uncovered (lifecycle_decode.go patch coverage 73%). Add table cases that feed a wrong type at every nested field position (release/install/upgrade/delete timeout, chart_hooks, wait, wait.strategy, wait.jobs, crds, on_failure, cleanup_on_failure, history/history.max, and a non-object release), so each decode error branch is executed. This lifts the helm-package release-policy code from ~82% to ~91% patch coverage.
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
4 participants
what
releasepolicy with release-wide defaults and operation-specificinstall,upgrade, anddeleteoverlays.values:key decodes to null, keep inline andvalues_filestemplate strings consistent, propagate top-level secrets, and honor command-local--mask=false.This is 2 of 4 in the native Helm lifecycle stack and is based on #2846:
why
validation
releasetree, including distinct install and upgrade timeouts and upgrade rollback plus cleanup policy.git diff --check, and generic fixture-name audit pass.references
docs/prd/native-helm-release-lifecycle.mdSummary by CodeRabbit