docs: define native Helm release lifecycle - #2846
docs: define native Helm release lifecycle#2846Mikhail Shirkov (shirkevich) wants to merge 11 commits into
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Mikhail Shirkov (shirkevich)
added
the
no-release
|
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:
📝 WalkthroughWalkthroughThe PRD defines configurable native Helm release lifecycle behavior, including configuration, resolution, execution semantics, command interfaces, validation, testing, implementation phases, and deferred work. ChangesNative Helm release lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 4
🤖 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 `@docs/prd/native-helm-release-lifecycle.md`:
- Line 5: Update the “Last Updated” metadata in the native Helm release
lifecycle PRD to the actual date this PRD was most recently changed, ensuring it
is not a future date relative to the review date.
- Line 348: Fix whitespace in docs/prd/native-helm-release-lifecycle.md by
replacing tab indentation in the Go example around lines 420-433 with spaces and
correcting the padding on the diagram line containing helm/dependent-release,
while preserving the diagram alignment and example formatting.
- Around line 360-373: Update the Helm action mapping documentation around
releaseLifecycle and the action mapping table to explicitly state whether
dry-run remains outside releaseLifecycle, while documenting its required
propagation for apply and delete. Extend the mapping to cover rollback and align
each install, upgrade, rollback, and uninstall command with its provider
operation and Helm action timeout recovery behavior.
- Around line 176-185: Update the lifecycle CLI examples in the PRD to use only
currently supported flag syntax, or first add the corresponding lifecycle flag
builders and tests under the existing Boolean flag contract. Ensure examples do
not prescribe `=false` forms unless those flags explicitly support consuming
boolean values, including `--rollback-on-failure`, `--wait-for-jobs`, and
`--disable-chart-hooks`.
🪄 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: 0e9e5e11-25a4-43b1-89db-7f1c138a2313
📒 Files selected for processing (1)
docs/prd/native-helm-release-lifecycle.md
2979415 to
1b7cf18
Compare
|
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2846 +/- ##
==========================================
+ Coverage 82.75% 82.76% +0.01%
==========================================
Files 1860 1860
Lines 180240 180240
==========================================
+ Hits 149150 149175 +25
+ Misses 23306 23283 -23
+ Partials 7784 7782 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/prd/native-helm-release-lifecycle.md`:
- Around line 375-390: The Context and Cancellation section must not promise
cancellation of in-flight Helm SDK actions unless implementation owns and waits
for their worker goroutines. Update the PRD’s Helm propagation and rollback
requirements to state that caller cancellation stops waiting and prevents or
cancels rollback attempts, while an already-running Install or Upgrade action
may continue; alternatively require explicit worker ownership and waiting before
returning.
- Line 482: Update the dry-run propagation requirement in the
command-to-provider path: map apply/deploy operations to Helm’s
Install.DryRunStrategy or Upgrade.DryRunStrategy fields, and map delete
operations to Uninstall.DryRun. Remove references to the non-existent
Install.DryRun and Upgrade.DryRun fields while keeping dry-run independent of
releaseLifecycle.
🪄 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: 0d02114b-9e50-4a64-8907-bbe880171e52
📒 Files selected for processing (1)
docs/prd/native-helm-release-lifecycle.md
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/prd/native-helm-release-lifecycle.md (1)
587-588: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a direct test for internal rollback mapping.
The Helm action table has a separate rollback row at Line 488, but the unit-test requirements at Lines 587-588 name only install, upgrade, and delete mappings.
Add a deterministic test that verifies internal rollback receives the documented timeout and wait configuration. Also verify that successful recovery still returns the original upgrade failure.
🤖 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 `@docs/prd/native-helm-release-lifecycle.md` around lines 587 - 588, Add a deterministic unit test for the internal rollback path described by the Helm action table, verifying rollback receives the documented timeout and wait configuration. Make the upgrade fail, confirm rollback succeeds with those mapped options, and assert the overall operation still returns the original upgrade error rather than the rollback result.
🤖 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.
Nitpick comments:
In `@docs/prd/native-helm-release-lifecycle.md`:
- Around line 587-588: Add a deterministic unit test for the internal rollback
path described by the Helm action table, verifying rollback receives the
documented timeout and wait configuration. Make the upgrade fail, confirm
rollback succeeds with those mapped options, and assert the overall operation
still returns the original upgrade error rather than the rollback result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 078e5902-8766-4dcd-b290-7ef67601cf81
📒 Files selected for processing (1)
docs/prd/native-helm-release-lifecycle.md
a743f12 to
7bb44fc
Compare
|
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 42 minutes. |
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/prd/native-helm-release-lifecycle.md`:
- Line 297: Update the cleanup_on_fail behavior and upgrade-architecture
descriptions to state that cleanup is applied when configured, independently of
rollback_on_failure. Ensure failed new resources are cleaned up with both
rollback enabled and disabled, and add coverage for both scenarios.
- Around line 291-300: Update the upgrade summary example to use the canonical
disable_chart_hooks and skip_crds fields, and remove install_crds because
skip_crds is not applicable to upgrades. Alternatively, change the example to an
install operation where skip_crds applies, or explicitly define any derived
report keys and their applicability.
- Around line 213-219: The release lifecycle documentation must not claim delete
remains bounded when timeout: 0s disables Helm’s uninstall timeout and the SDK
lacks context propagation. Update the timeout requirements and corresponding
guidance around the timeout behavior section and the referenced delete section
to either reject zero for delete, define a separate bounded caller cancellation,
or explicitly document and test the unbounded-wait limitation.
- Around line 360-373: Update the “Timeout Semantics” documentation to
explicitly exclude installed CRD installation and readiness waiting from the
release timeout guarantee, or define a separate CRD-readiness timeout if the
implementation supports one. Clarify the distinction from Helm’s
install/upgrade/delete operation timeout and add a test using the existing CRD
fixture to verify the documented behavior.
- Around line 275-285: Update the native Helm boolean flag documentation around
the precedence and flag table to define how users explicitly set every lifecycle
boolean to false, including supported negative flags or an explicit =false form.
Ensure the documented behavior allows command-line false values to override
stack-level true settings, and add coverage for each Boolean lifecycle flag.
🪄 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: 44fa9626-d336-4d56-8531-70386aa28b7d
📒 Files selected for processing (1)
docs/prd/native-helm-release-lifecycle.md
|
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. |
|
CodeRabbit (@coderabbitai) full review |
0e744ac to
a46eb57
Compare
|
Erik Osterman (Cloud Posse) (@osterman), the two |
|
CodeRabbit (@coderabbitai) here's what I propose. What do you think? The shape of the lifecycle configurationI want to agree on the shape of this configuration before we merge the PRD. The current design puts ten lifecycle fields directly on the component. These fields are siblings of The PRD shows this problem in two places:
The field names show the same problem. We must use the name Terminology and structure are differentThe Design Principles section says that Helm 4 terminology is canonical. I agree with this rule for the names. I do not agree with this rule for the structure. Helm has a flat structure because Helm is a command-line program. Command-line flags cannot have a hierarchy. Atmos configuration is YAML, and YAML has a hierarchy. We can keep the Helm names. We can also reject the Helm structure. Please make this difference clear in the Design Principles section. The proposed structureFlux solves the same problem in the HelmRelease resource. Flux groups the fields by operation. Each operation has its own timeout, hook control, and failure policy. Flux keeps the release-wide fields, such as I propose the same approach for Atmos: components:
helm:
demo-api:
chart: oci://registry.invalid/charts/demo-api
version: 1.2.3
namespace: demo
release:
# Release-wide defaults. Each operation section inherits these values.
timeout: 10m
chart_hooks: true
wait:
strategy: watcher # watcher | hookOnly | legacy
jobs: true
history:
max: 10
install:
crds: create # create | replace | skip
on_failure: uninstall # uninstall | keep
upgrade:
timeout: 30m
on_failure: rollback # rollback | uninstall | keep
cleanup_on_failure: true
rollback:
timeout: 5m
cleanup_on_failure: true
delete:
timeout: 5m
chart_hooks: falseStack-level defaults keep the usual component short: helm:
release:
timeout: 10m
wait:
strategy: watcher
history:
max: 10
upgrade:
on_failure: rollback
components:
helm:
demo-api:
chart: charts/demo-api
namespace: demo
release:
upgrade:
timeout: 30mThis structure is still a component-owned field. It does not use The effects of this change
Three questions for the PRD
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
prd="$(fd -a '^native-helm-release-lifecycle\.md$' docs 2>/dev/null | head -n1)"
if [ -z "$prd" ]; then
echo "PRD file not found"
exit 0
fi
echo "== PRD =="
echo "$prd"
echo
echo "== Outline =="
ast-grep outline "$prd" --lang markdown --items all 2>/dev/null || true
echo
echo "== Relevant headings and configuration terms =="
rg -n -i -C 3 \
'^(#{1,6} )|cleanup_on_fail|skip_crds|disable_chart_hooks|on_failure|atomic|wait|precedence|inherit|rollback|timeout|max_history|history|flag|install|upgrade|uninstall|delete|CRD|Helm.*4|action\.' \
"$prd"
echo
echo "== Repository references to lifecycle config or Helm action options =="
rg -n -i -C 2 \
'cleanup_on_fail|skip_crds|disable_chart_hooks|on_failure|native Helm|HelmRelease|WaitStrategy|Upgrade.*Options|Install.*Options|Rollback.*Options|Uninstall.*Options' \
-g '!docs/prd/native-helm-release-lifecycle.md' \
. || trueLength of output: 201283 🏁 Script executed: #!/bin/bash
set -euo pipefail
prd="docs/prd/native-helm-release-lifecycle.md"
echo "== PRD headings =="
rg -n '^#{1,6} ' "$prd"
echo
echo "== PRD: configuration through precedence =="
sed -n '1,360p' "$prd"
echo
echo "== PRD: operation mapping, compatibility, and implementation sections =="
sed -n '360,760p' "$prd"
echo
echo "== Native Helm inherited field keys =="
sed -n '285,345p' internal/exec/stack_processor_process_stacks_helpers_extraction.go
echo
echo "== Current Helm action configuration =="
sed -n '55,190p' pkg/component/helm/client.go
echo
echo "== Apply operation selection =="
sed -n '175,225p' pkg/component/helm/executor.goLength of output: 57102
It is a better public contract than the current flat fields. It makes the operation scope visible in configuration and enforceable in the schema. It also gives deferred fields stable locations. I recommend these decisions:
One additional schema rule is needed: each operation object must reject unknown keys with This direction should replace the flat contract before merge. The only part I would defer is the effective You are interacting with an AI system. |
|
Erik Osterman (Cloud Posse) (@osterman), I updated the PRD through 3cddb26 to adopt the proposed
The contract now has release-wide defaults plus Flux provides the relevant operation-scoped controller precedent. The POC-specific need is represented generically as a 60-minute first install, 10-minute cached upgrade, and 5-minute delete. Rendering, values, hooks, Prettier and |
|
Erik Osterman (Cloud Posse) (@osterman), the revised release-policy proposal is ready for rereview at 3cddb26. It incorporates your hierarchical |
a89d23b to
63475cf
Compare
2 participants
what
This is 1 of 4 in the native Helm lifecycle stack:
why
validation
references
docs/prd/native-helm-release-lifecycle.mdSummary by CodeRabbit