feat: propagate component execution context - #2848
feat: propagate component execution context#2848Mikhail Shirkov (shirkevich) wants to merge 23 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 |
|
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 change propagates command contexts through component, lifecycle, graph, Ansible, and Helm execution. Helm supports optional dependency updates, hook manifests, and cancellation-aware release operations. Command masking now honors the nearest changed local flag. ChangesContext propagation and execution
Command masking reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CobraCommand
participant HelmExecutor
participant ChartLoader
participant HelmRelease
participant ExternalTarget
CobraCommand->>HelmExecutor: pass caller context and dependency-update
HelmExecutor->>ChartLoader: load and validate chart
ChartLoader->>ChartLoader: update missing dependencies when enabled
HelmExecutor->>HelmRelease: render, apply, diff, or delete with context
HelmExecutor->>ExternalTarget: deliver rendered manifests with context
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/component/helm/client.go (1)
172-193: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel Helm uninstalls during waits.
deleteReleaseonly checksctx.Err()before and afterclient.Run, so cancellation does not stop the Helm action once it starts.action.Uninstall.Rundoes not accept a context, butWaitOptionscan pass contexts to Helm waits. Setclient.WaitOptionswithkube.WithWaitContext(ctx)andkube.WithWaitForDeleteMethodContext(ctx)for the uninstall waiter.🤖 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/client.go` around lines 172 - 193, The deleteRelease function must propagate ctx cancellation into the Helm uninstall waits, not only check it before and after client.Run. Configure client.WaitOptions after creating the uninstall client with kube.WithWaitContext(ctx) and kube.WithWaitForDeleteMethodContext(ctx), preserving the existing lifecycle configuration and error handling.
🤖 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/graph.go`:
- Around line 67-69: Update ExecuteGraph’s executeGraphNode error path to check
ctx.Err() after an active node returns; when cancellation is present, wrap the
returned error with errUtils.ErrGraphExecutionCanceled while preserving the
original error for errors.Is matching. Extend the relevant graph tests to assert
both ErrComponentExecutionFailed and ErrGraphExecutionCanceled.
---
Outside diff comments:
In `@pkg/component/helm/client.go`:
- Around line 172-193: The deleteRelease function must propagate ctx
cancellation into the Helm uninstall waits, not only check it before and after
client.Run. Configure client.WaitOptions after creating the uninstall client
with kube.WithWaitContext(ctx) and kube.WithWaitForDeleteMethodContext(ctx),
preserving the existing lifecycle configuration and error handling.
🪄 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: 223c7a47-aaa6-4dc8-9114-7847a2bb6c86
📒 Files selected for processing (16)
cmd/helm/helm.gocmd/helm/helm_test.gopkg/component/graph.gopkg/component/graph_test.gopkg/component/helm/client.gopkg/component/helm/client_lifecycle_test.gopkg/component/helm/client_test.gopkg/component/helm/diff_test.gopkg/component/helm/executor.gopkg/component/helm/executor_bulk.gopkg/component/helm/executor_extra_test.gopkg/component/helm/executor_test.gopkg/component/helm/provision.gopkg/component/helm/provision_test.gopkg/component/provider.gopkg/component/provider_test.go
b71f180 to
277e2f0
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 33 minutes. |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (77.94%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## codex/helm-lifecycle-actions #2848 +/- ##
================================================================
+ Coverage 82.75% 82.76% +0.01%
================================================================
Files 1861 1862 +1
Lines 180834 180982 +148
================================================================
+ Hits 149649 149792 +143
+ Misses 23361 23357 -4
- Partials 7824 7833 +9
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 `@pkg/component/helm/client_lifecycle_test.go`:
- Around line 145-154: Extend TestApplyReleaseHonorsCanceledContext by querying
getDeployedManifest after applyRelease returns and asserting the resulting
manifest is empty, while preserving the existing context.Canceled error
assertion. Use the test’s existing action context and release identifiers to
verify that cancellation leaves no deployed release.
In `@pkg/component/provider.go`:
- Around line 49-51: Update every unkeyed component.ExecutionContext literal
passed to provider.Execute in the affected command implementations to keyed
fields, preserving each existing positional value and populating Context through
the established GoContext propagation path. Keep the Context field in
ExecutionContext, and document the struct as keyed-only for external callers so
future additions cannot silently misalign values.
🪄 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: fe627d87-3556-476c-8ca3-db334bcec369
📒 Files selected for processing (16)
cmd/helm/helm.gocmd/helm/helm_test.gopkg/component/graph.gopkg/component/graph_test.gopkg/component/helm/client.gopkg/component/helm/client_lifecycle_test.gopkg/component/helm/client_test.gopkg/component/helm/diff_test.gopkg/component/helm/executor.gopkg/component/helm/executor_bulk.gopkg/component/helm/executor_extra_test.gopkg/component/helm/executor_test.gopkg/component/helm/provision.gopkg/component/helm/provision_test.gopkg/component/provider.gopkg/component/provider_test.go
2d1f2df to
bfe167e
Compare
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 47 minutes. |
bfe167e to
e12ad25
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 9 minutes. |
6c74f34 to
ad9b431
Compare
1 participant
what
--dependency-updatesupport to chart-loading operations without hidden network access by default.This is 3 of 4 in the native Helm lifecycle stack and is based on #2847:
why
context.Background(), preventing signals and scheduler cancellation from consistently reaching active operations.validation
git diff --checkpasses.references
docs/prd/native-helm-release-lifecycle.md