Support deploying add-on Helm charts alongside central#242
Open
mclasmeier wants to merge 12 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds configurable Helm chart add-ons sourced from remote repositories or StackRox checkouts, with lifecycle management tied to Central deployment and teardown. It also introduces reusable Helm operations, configuration restoration during teardown, integration tests, E2E coverage, and documentation. ChangesHelm chart add-on lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 8
🧹 Nitpick comments (2)
internal/deployer/addons_stackrox_helm_chart.go (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error.
"invokeod" should be spelled "invoked".
♻️ Proposed fix
if !env.IsInStackroxRepository(addOnCfg.log) { addOnCfg.log.Errorf("the Helm chart add-on %q uses stackroxRepoHelmChart but roxie is not running from a stackrox checkout", name) - return nil, errors.New("not invokeod in StackRox repository") + return nil, errors.New("not invoked in StackRox repository") }🤖 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 `@internal/deployer/addons_stackrox_helm_chart.go` around lines 13 - 17, In StackRoxRepoHelmChartAddOn.New, correct the error message text from “invokeod” to “invoked” while leaving the surrounding validation and return behavior unchanged.tests/e2e/addons_test.go (1)
37-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRegister teardown with
t.Cleanupafter deployment.A fatal verification failure currently skips lines 44–48 and leaves Central and its add-on installed, potentially contaminating subsequent E2E tests. Register failure-safe teardown and disable it after the explicit teardown succeeds.
🤖 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 `@tests/e2e/addons_test.go` around lines 37 - 48, Register a t.Cleanup callback immediately after the Central deployment succeeds to run the existing teardown and add-on cleanup logic when verification fails. Track whether explicit teardown completed successfully, and have the cleanup callback return without repeating teardown once that flag is set.
🤖 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 `@go.mod`:
- Around line 31-87: Update the indirect dependencies
github.com/containerd/containerd and oras.land/oras-go/v2 to their latest
patched versions, then run go mod tidy to synchronize go.mod and go.sum.
Preserve the existing dependency declarations and update any related transitive
versions required by the resolved module graph.
In `@internal/deployer/addons.go`:
- Around line 24-38: Update deployAddOns and the corresponding teardown add-on
method to return aggregated errors instead of only logging failures; continue
attempting every add-on operation, collect each lifecycle error, and return the
combined result. Update Deploy and teardownCentral to propagate and handle these
returned errors so they do not report success or delete Central when add-on
lifecycle operations fail.
In `@internal/deployer/deployer.go`:
- Around line 455-459: Update the teardown flow around ResolveEnabledAddOns to
discover installed Helm releases in the Central namespace using the roxie-addon-
prefix instead of resolving the current configuration. Pass the discovered
release names to teardownAddOns so cleanup also works with --skip-user-config
and removes all matching installed add-ons.
- Around line 312-319: Guard the prepareNamespace and ResolveEnabledAddOns calls
in the deployment flow with components.IncludesCentral(), placing them after the
operator-only return and inside the Central-specific path. Ensure operator-only
and secured-cluster deployments skip Central namespace creation and irrelevant
add-on resolution while preserving the existing Central setup behavior.
In `@internal/helm/helm_integration_test.go`:
- Line 28: Update all affected Install and Uninstall calls in
internal/helm/helm_integration_test.go: lines 28-28, 36-36, and 67-67 must pass
false as the verbose argument, and lines 44-44 and 52-52 must pass false before
releaseName and namespace.
In `@internal/helm/helm.go`:
- Around line 126-134: Prevent nil logger panics throughout
internal/helm/helm.go by guarding every listed log invocation: wrap verbose
logging blocks around lines 126-134, 157-159, 166-169, 176-179, 189-191,
198-201, and 213-215 with log != nil && verbose, and guard Warningf at 138 and
Infof at 299 with log != nil. Ensure the helpers.LogMultilineYaml call in the
166-169 block is covered by the same guard.
- Around line 96-118: Update executeHelmActionWithRetries to accept ctx as its
first argument, and update its callers in Install, Uninstall, and ListByPrefix.
Stop retrying immediately when ctx is canceled or its deadline expires, make all
retry logging safe when log is nil, and restructure the final-attempt handling
so the loop exits naturally without an unreachable return while preserving the
final wrapped error.
In `@tests/e2e/addons_test.go`:
- Around line 22-27: Update the availableAddOns fixture in the add-ons
end-to-end configuration to use the name-keyed map contract expected by
CentralConfig, nesting the test-chart entry under its key while preserving its
helmChart repository, chart, and version values.
---
Nitpick comments:
In `@internal/deployer/addons_stackrox_helm_chart.go`:
- Around line 13-17: In StackRoxRepoHelmChartAddOn.New, correct the error
message text from “invokeod” to “invoked” while leaving the surrounding
validation and return behavior unchanged.
In `@tests/e2e/addons_test.go`:
- Around line 37-48: Register a t.Cleanup callback immediately after the Central
deployment succeeds to run the existing teardown and add-on cleanup logic when
verification fails. Track whether explicit teardown completed successfully, and
have the cleanup callback return without repeating teardown once that flag is
set.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: e9d29865-6c77-42c1-9e03-46a1cc0366e5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
README.mdcmd/teardown.gogo.modinternal/deployer/addons.gointernal/deployer/addons_helm_chart.gointernal/deployer/addons_stackrox_helm_chart.gointernal/deployer/addons_test.gointernal/deployer/config.gointernal/deployer/config_addons.gointernal/deployer/deploy_via_operator.gointernal/deployer/deployer.gointernal/env/env.gointernal/helm/helm.gointernal/helm/helm_integration_test.gointernal/helm/testdata/minimal-test-chart/Chart.yamlinternal/helm/testdata/minimal-test-chart/templates/configmap.yamlinternal/helpers/helpers.gotests/e2e/addons_test.gotests/e2e/helpers.go
💤 Files with no reviewable changes (1)
- internal/deployer/deploy_via_operator.go
Comment on lines
+312
to
+319
| if err := d.prepareNamespace(ctx, d.config.Central.Namespace, needPullSecrets); err != nil { | ||
| return fmt.Errorf("failed to prepare namespace: %w", err) | ||
| } | ||
|
|
||
| enabledAddOns, err := d.ResolveEnabledAddOns() | ||
| if err != nil { | ||
| return fmt.Errorf("resolving enabled add-ons: %w", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run Central-specific setup only when Central is requested.
These operations run before the operator-only return and without checking components.IncludesCentral(). Consequently, operator-only or secured-cluster deployments can create the Central namespace or fail because of irrelevant add-on configuration. Move this setup into the Central path.
🤖 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 `@internal/deployer/deployer.go` around lines 312 - 319, Guard the
prepareNamespace and ResolveEnabledAddOns calls in the deployment flow with
components.IncludesCentral(), placing them after the operator-only return and
inside the Central-specific path. Ensure operator-only and secured-cluster
deployments skip Central namespace creation and irrelevant add-on resolution
while preserving the existing Central setup behavior.
Comment on lines
+455
to
+459
| enabledAddOns, err := d.ResolveEnabledAddOns() | ||
| if err != nil { | ||
| return fmt.Errorf("resolving enabled add-ons: %w", err) | ||
| } | ||
| d.teardownAddOns(ctx, enabledAddOns) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Discover installed releases instead of resolving the current configuration.
Teardown may run with --skip-user-config and no original config, as the E2E flow does. ResolveEnabledAddOns() then returns no add-ons, leaving roxie-addon-* releases installed. Use Helm prefix discovery in the Central namespace and uninstall the discovered releases.
🤖 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 `@internal/deployer/deployer.go` around lines 455 - 459, Update the teardown
flow around ResolveEnabledAddOns to discover installed Helm releases in the
Central namespace using the roxie-addon- prefix instead of resolving the current
configuration. Pass the discovered release names to teardownAddOns so cleanup
also works with --skip-user-config and removes all matching installed add-ons.
| Namespace: namespace, | ||
| } | ||
|
|
||
| err := Install(ctx, nil, opts) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Compilation errors due to missing verbose argument.
The function signatures for Install and Uninstall in helm.go require a verbose bool argument, but the integration tests were not updated to supply it, which breaks the build.
internal/helm/helm_integration_test.go#L28-L28: Update call toInstall(ctx, nil, false, opts).internal/helm/helm_integration_test.go#L36-L36: Update call toInstall(ctx, nil, false, opts).internal/helm/helm_integration_test.go#L44-L44: Update call toUninstall(ctx, nil, false, releaseName, namespace).internal/helm/helm_integration_test.go#L52-L52: Update call toUninstall(ctx, nil, false, releaseName, namespace).internal/helm/helm_integration_test.go#L67-L67: Update call toInstall(ctx, nil, false, opts).
📍 Affects 1 file
internal/helm/helm_integration_test.go#L28-L28(this comment)internal/helm/helm_integration_test.go#L36-L36internal/helm/helm_integration_test.go#L44-L44internal/helm/helm_integration_test.go#L52-L52internal/helm/helm_integration_test.go#L67-L67
🤖 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 `@internal/helm/helm_integration_test.go` at line 28, Update all affected
Install and Uninstall calls in internal/helm/helm_integration_test.go: lines
28-28, 36-36, and 67-67 must pass false as the verbose argument, and lines 44-44
and 52-52 must pass false before releaseName and namespace.
Comment on lines
+96
to
+118
| func executeHelmActionWithRetries(log *logger.Logger, actionName string, helmAction func() error) error { | ||
| var lastErr error | ||
| for attempt := 1; attempt <= maxAttempts; attempt++ { | ||
| if attempt > 1 { | ||
| waitTime := time.Duration(attempt) * retryDelay | ||
| log.Infof("Retrying helm %s (attempt %d/%d) after %v...", actionName, attempt, maxAttempts, waitTime) | ||
| time.Sleep(waitTime) | ||
| } | ||
|
|
||
| err := helmAction() | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| lastErr = err | ||
|
|
||
| if !isRetryable(err) || attempt == maxAttempts { | ||
| return fmt.Errorf("helm %s failed: %w", actionName, err) | ||
| } | ||
|
|
||
| log.Warningf("Transient error during helm %s: %v", actionName, err) | ||
| } | ||
| return fmt.Errorf("helm %s failed after %d attempts: %w", actionName, maxAttempts, lastErr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix dead code, context exhaustion delay, and panic risks.
There are three issues in this retry loop:
- Unreachable return / Dead code: Returning early inside the loop when
attempt == maxAttemptsprevents the loop from completing naturally, rendering the final return statement (helm %s failed after %d attempts) unreachable dead code. - Context exhaustion delay:
"context deadline exceeded"is inretryableErrors. If the parentctxexpires, this loop blindly sleeps and retries, delaying the inevitable failure. - Panic risk:
logis dereferenced directly, which will panic because tests passnilfor it (as evidenced byhelm_integration_test.go).
Consider passing ctx to this function to enable fast-fail, and applying these fixes:
(Note: Be sure to update the calls to executeHelmActionWithRetries in Install, Uninstall, and ListByPrefix to pass ctx as the first argument).
🛠️ Proposed fixes for the retry loop
-func executeHelmActionWithRetries(log *logger.Logger, actionName string, helmAction func() error) error {
+func executeHelmActionWithRetries(ctx context.Context, log *logger.Logger, actionName string, helmAction func() error) error {
var lastErr error
for attempt := 1; attempt <= maxAttempts; attempt++ {
+ if err := ctx.Err(); err != nil {
+ return fmt.Errorf("helm %s aborted: %w", actionName, err)
+ }
+
if attempt > 1 {
waitTime := time.Duration(attempt) * retryDelay
- log.Infof("Retrying helm %s (attempt %d/%d) after %v...", actionName, attempt, maxAttempts, waitTime)
+ if log != nil {
+ log.Infof("Retrying helm %s (attempt %d/%d) after %v...", actionName, attempt, maxAttempts, waitTime)
+ }
time.Sleep(waitTime)
}
err := helmAction()
if err == nil {
return nil
}
lastErr = err
- if !isRetryable(err) || attempt == maxAttempts {
+ if !isRetryable(err) {
return fmt.Errorf("helm %s failed: %w", actionName, err)
}
+
+ if attempt == maxAttempts {
+ break
+ }
- log.Warningf("Transient error during helm %s: %v", actionName, err)
+ if log != nil {
+ log.Warningf("Transient error during helm %s: %v", actionName, err)
+ }
}
return fmt.Errorf("helm %s failed after %d attempts: %w", actionName, maxAttempts, lastErr)
}🤖 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 `@internal/helm/helm.go` around lines 96 - 118, Update
executeHelmActionWithRetries to accept ctx as its first argument, and update its
callers in Install, Uninstall, and ListByPrefix. Stop retrying immediately when
ctx is canceled or its deadline expires, make all retry logging safe when log is
nil, and restructure the final-attempt handling so the loop exits naturally
without an unreachable return while preserving the final wrapped error.
Comment on lines
+126
to
+134
| if verbose { | ||
| if opts.ChartPath != "" { | ||
| log.Dimf("installing Helm chart from directory %q as release %s into namespace %s", | ||
| opts.ChartPath, opts.ReleaseName, opts.Namespace) | ||
| } else { | ||
| log.Dimf("installing Helm chart %s/%s:%s as release %s into namespace %s", | ||
| opts.RepoURL, opts.ChartName, opts.ChartVersion, opts.ReleaseName, opts.Namespace) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Missing log != nil guards before logger calls.
Throughout this file, log is dereferenced inside verbose checks or directly (e.g., log.Dimf, log.Warningf, helpers.LogMultilineYaml) without verifying that log != nil. Because nil is intentionally passed from tests, these lines will panic if evaluated. Please ensure all log invocations are guarded.
internal/helm/helm.go#L126-L134: Guard this block withif log != nil && verbose {.internal/helm/helm.go#L138-L138: Guardlog.Warningfwithif log != nil {.internal/helm/helm.go#L157-L159: Guard this block withif log != nil && verbose {.internal/helm/helm.go#L166-L169: Guard this block withif log != nil && verbose {. (Note:helpers.LogMultilineYaml(log, ...)will also panic).internal/helm/helm.go#L176-L179: Guard this block withif log != nil && verbose {.internal/helm/helm.go#L189-L191: Guard this block withif log != nil && verbose {.internal/helm/helm.go#L198-L201: Guard this block withif log != nil && verbose {.internal/helm/helm.go#L213-L215: Guard this block withif log != nil && verbose {.internal/helm/helm.go#L299-L299: Guardlog.Infofwithif log != nil {.
📍 Affects 1 file
internal/helm/helm.go#L126-L134(this comment)internal/helm/helm.go#L138-L138internal/helm/helm.go#L157-L159internal/helm/helm.go#L166-L169internal/helm/helm.go#L176-L179internal/helm/helm.go#L189-L191internal/helm/helm.go#L198-L201internal/helm/helm.go#L213-L215internal/helm/helm.go#L299-L299
🤖 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 `@internal/helm/helm.go` around lines 126 - 134, Prevent nil logger panics
throughout internal/helm/helm.go by guarding every listed log invocation: wrap
verbose logging blocks around lines 126-134, 157-159, 166-169, 176-179, 189-191,
198-201, and 213-215 with log != nil && verbose, and guard Warningf at 138 and
Infof at 299 with log != nil. Ensure the helpers.LogMultilineYaml call in the
166-169 block is covered by the same guard.
mclasmeier
force-pushed
the
mc/helm-chart-add-ons
branch
from
469dc6c to
6575e1b
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
(Makes testing of add-ons faster.)
The add-ons system needs to locate paths relative to the stackrox repository root. Make this existing helper public so it can be called from outside the env package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wraps the Helm v3 SDK to provide Install (upgrade-install), Uninstall, and ListByPrefix operations. Install checks release history to decide between a fresh install and an upgrade, making it idempotent. All operations include retry logic for transient network errors, consistent with the retry approach used elsewhere in roxie. Test the full Helm lifecycle against a real cluster using a minimal local chart that deploys a single ConfigMap: install, idempotent re-install, list, uninstall, and uninstall of an already-removed release. Also verifies that Values and ValuesMap are applied correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Test deploying Central with a kube-state-metrics add-on, verifying the Helm release is created during deploy and removed during teardown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce AddOnDefinition, StackRoxRepoHelmChart, and HelmChartRef types to describe Helm-chart-based add-ons. Add AddOns and AvailableAddOns fields to CentralConfig so users can declare and toggle add-ons via config files or CLI flags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement resolveAddOns to match enabled switches against available definitions, validate names and release name length, and resolve chart sources (stackrox repo path or remote Helm repo). Add deployAddOns and teardownAddOns methods with values file loading and envsubst support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Call deployAddOns after Central becomes ready during deployment, and teardownAddOns before deleting Central resources during teardown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- fetch manifest from cluster and use that as configuration base
mclasmeier
force-pushed
the
mc/helm-chart-add-ons
branch
from
6575e1b to
171705f
Compare
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 `@internal/component/component.go`:
- Around line 65-68: Update the Teardown flow to use teardownAddOns instead of
deployAddOns when handling add-ons, and add an explicit component.AddOns case to
its switch so standalone AddOns teardown completes without reinstalling or
reaching the unknown-component error.
In `@internal/deployer/addons.go`:
- Around line 46-63: Update teardownAddOns so the Warningf call is used only for
optional add-on teardown failures; make it the else branch of the non-optional
Errorf condition, ensuring each failure emits exactly one log message.
In `@internal/helm/helm.go`:
- Around line 103-125: Update executeHelmActionWithRetries to remove the attempt
== maxAttempts early return so the final “failed after %d attempts” path is
reachable, while preserving immediate returns for non-retryable errors. Before
sleeping or retrying, check helmCtx.Ctx.Err() and return the
cancellation/deadline error without further attempts; also exclude context
deadline errors from retryableErrors. Guard helmCtx.Log before calling Infof or
Warningf.
- Around line 137-232: Guard every logging call in doInstall, doUpgrade,
doUninstall, and BuildDependencies before dereferencing helmCtx.Log or log.
Reuse the nil-safe pattern already established in newActionConfig, ensuring
normal logging remains unchanged when a logger is provided and nil logger
contexts complete without panicking.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 454b10a4-bda9-45a7-a9ba-c762da6d7e26
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
cmd/teardown.gogo.modinternal/component/component.gointernal/deployer/addons.gointernal/deployer/addons_helm_chart.gointernal/deployer/addons_stackrox_helm_chart.gointernal/deployer/addons_test.gointernal/deployer/config.gointernal/deployer/config_addons.gointernal/deployer/deploy_via_operator.gointernal/deployer/deployer.gointernal/env/env.gointernal/helm/helm.gointernal/helm/helm_integration_test.gointernal/helm/testdata/minimal-test-chart/Chart.yamlinternal/helm/testdata/minimal-test-chart/templates/configmap.yamltests/e2e/addons_test.gotests/e2e/helpers.go
💤 Files with no reviewable changes (1)
- internal/deployer/deploy_via_operator.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/helm/testdata/minimal-test-chart/Chart.yaml
- tests/e2e/addons_test.go
- internal/deployer/config_addons.go
- internal/deployer/config.go
- go.mod
- tests/e2e/helpers.go
- internal/deployer/addons_test.go
- internal/env/env.go
- cmd/teardown.go
- internal/deployer/deployer.go
Comment on lines
+65
to
+68
| func (c Component) IncludesAddOns() bool { | ||
| // Aligned with the central lifecycle, but can also be selected explicitly. | ||
| return c.IncludesCentral() || c == AddOns | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how Teardown actually handles the AddOns component.
rg -n -B3 -A40 'func \(d \*Deployer\) Teardown' internal/deployer/deployer.goRepository: stackrox/roxie
Length of output: 1539
🏁 Script executed:
#!/bin/bash
sed -n '410,480p' internal/deployer/deployer.goRepository: stackrox/roxie
Length of output: 2182
Fix add-ons teardown in Teardown components.IncludesAddOns() still calls deployAddOns during teardown, so a standalone AddOns run re-installs add-ons and then falls through to unknown component: AddOns. Call teardownAddOns here and add an explicit case component.AddOns: in the switch.
🤖 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 `@internal/component/component.go` around lines 65 - 68, Update the Teardown
flow to use teardownAddOns instead of deployAddOns when handling add-ons, and
add an explicit component.AddOns case to its switch so standalone AddOns
teardown completes without reinstalling or reaching the unknown-component error.
Comment on lines
+46
to
+63
| func (d *Deployer) teardownAddOns(ctx context.Context, addOns []AddOn) { | ||
| if len(addOns) == 0 { | ||
| return | ||
| } | ||
|
|
||
| d.logger.Infof("Tearing down %d add-on(s)...", len(addOns)) | ||
|
|
||
| for _, addon := range addOns { | ||
| if err := addon.Teardown(ctx); err != nil { | ||
| if !addon.IsOptional() { | ||
| d.logger.Errorf("Failed to tear down non-optional add-on %q: %v", addon.Name(), err) | ||
| } | ||
| d.logger.Warningf("Failed to tear down add-on %q: %v", addon.Name(), err) | ||
| } else { | ||
| d.logger.Successf("Add-on %q torn down", addon.Name()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Redundant duplicate log line for non-optional add-on teardown failures.
When a non-optional add-on fails to tear down, both Errorf and the unconditional Warningf fire for the same failure, producing two log lines describing the same event. Use an else so only one message is emitted per outcome.
🛠️ Proposed fix
for _, addon := range addOns {
if err := addon.Teardown(ctx); err != nil {
if !addon.IsOptional() {
d.logger.Errorf("Failed to tear down non-optional add-on %q: %v", addon.Name(), err)
+ } else {
+ d.logger.Warningf("Failed to tear down add-on %q: %v", addon.Name(), err)
}
- d.logger.Warningf("Failed to tear down add-on %q: %v", addon.Name(), err)
} else {
d.logger.Successf("Add-on %q torn down", addon.Name())
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (d *Deployer) teardownAddOns(ctx context.Context, addOns []AddOn) { | |
| if len(addOns) == 0 { | |
| return | |
| } | |
| d.logger.Infof("Tearing down %d add-on(s)...", len(addOns)) | |
| for _, addon := range addOns { | |
| if err := addon.Teardown(ctx); err != nil { | |
| if !addon.IsOptional() { | |
| d.logger.Errorf("Failed to tear down non-optional add-on %q: %v", addon.Name(), err) | |
| } | |
| d.logger.Warningf("Failed to tear down add-on %q: %v", addon.Name(), err) | |
| } else { | |
| d.logger.Successf("Add-on %q torn down", addon.Name()) | |
| } | |
| } | |
| } | |
| func (d *Deployer) teardownAddOns(ctx context.Context, addOns []AddOn) { | |
| if len(addOns) == 0 { | |
| return | |
| } | |
| d.logger.Infof("Tearing down %d add-on(s)...", len(addOns)) | |
| for _, addon := range addOns { | |
| if err := addon.Teardown(ctx); err != nil { | |
| if !addon.IsOptional() { | |
| d.logger.Errorf("Failed to tear down non-optional add-on %q: %v", addon.Name(), err) | |
| } else { | |
| d.logger.Warningf("Failed to tear down add-on %q: %v", addon.Name(), err) | |
| } | |
| } else { | |
| d.logger.Successf("Add-on %q torn down", addon.Name()) | |
| } | |
| } | |
| } |
🤖 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 `@internal/deployer/addons.go` around lines 46 - 63, Update teardownAddOns so
the Warningf call is used only for optional add-on teardown failures; make it
the else branch of the non-optional Errorf condition, ensuring each failure
emits exactly one log message.
Comment on lines
+103
to
+125
| func executeHelmActionWithRetries(helmCtx HelmCtx, actionName string, helmAction func(helmCtx HelmCtx) error) error { | ||
| var lastErr error | ||
| for attempt := 1; attempt <= maxAttempts; attempt++ { | ||
| if attempt > 1 { | ||
| waitTime := time.Duration(attempt) * retryDelay | ||
| helmCtx.Log.Infof("Retrying helm %s (attempt %d/%d) after %v...", actionName, attempt, maxAttempts, waitTime) | ||
| time.Sleep(waitTime) | ||
| } | ||
|
|
||
| err := helmAction(helmCtx) | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| lastErr = err | ||
|
|
||
| if !isRetryable(err) || attempt == maxAttempts { | ||
| return fmt.Errorf("helm %s failed: %w", actionName, err) | ||
| } | ||
|
|
||
| helmCtx.Log.Warningf("Transient error during helm %s: %v", actionName, err) | ||
| } | ||
| return fmt.Errorf("helm %s failed after %d attempts: %w", actionName, maxAttempts, lastErr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Retry loop still has unreachable final error, no fast-fail on context cancellation, and unguarded logger calls.
This is functionally the same retry loop flagged previously, just refactored to take HelmCtx instead of separate args — none of the three issues were fixed:
- Line 118 (
attempt == maxAttempts) still returns early inside the OR, making line 124's"failed after %d attempts"message unreachable dead code. "context deadline exceeded"remains inretryableErrors(line 49) and the loop never checkshelmCtx.Ctx.Err(), so an already-canceled/expired parent context still sleeps and retries pointlessly before failing.helmCtx.Log.Infof(...)(line 108) andhelmCtx.Log.Warningf(...)(line 122) dereferenceLogunguarded — panics ifLogis nil.
🛠️ Proposed fix
func executeHelmActionWithRetries(helmCtx HelmCtx, actionName string, helmAction func(helmCtx HelmCtx) error) error {
var lastErr error
for attempt := 1; attempt <= maxAttempts; attempt++ {
+ if err := helmCtx.Ctx.Err(); err != nil {
+ return fmt.Errorf("helm %s aborted: %w", actionName, err)
+ }
+
if attempt > 1 {
waitTime := time.Duration(attempt) * retryDelay
- helmCtx.Log.Infof("Retrying helm %s (attempt %d/%d) after %v...", actionName, attempt, maxAttempts, waitTime)
+ if helmCtx.Log != nil {
+ helmCtx.Log.Infof("Retrying helm %s (attempt %d/%d) after %v...", actionName, attempt, maxAttempts, waitTime)
+ }
time.Sleep(waitTime)
}
err := helmAction(helmCtx)
if err == nil {
return nil
}
lastErr = err
- if !isRetryable(err) || attempt == maxAttempts {
+ if !isRetryable(err) {
return fmt.Errorf("helm %s failed: %w", actionName, err)
}
+ if attempt == maxAttempts {
+ break
+ }
- helmCtx.Log.Warningf("Transient error during helm %s: %v", actionName, err)
+ if helmCtx.Log != nil {
+ helmCtx.Log.Warningf("Transient error during helm %s: %v", actionName, err)
+ }
}
return fmt.Errorf("helm %s failed after %d attempts: %w", actionName, maxAttempts, lastErr)
}🤖 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 `@internal/helm/helm.go` around lines 103 - 125, Update
executeHelmActionWithRetries to remove the attempt == maxAttempts early return
so the final “failed after %d attempts” path is reachable, while preserving
immediate returns for non-retryable errors. Before sleeping or retrying, check
helmCtx.Ctx.Err() and return the cancellation/deadline error without further
attempts; also exclude context deadline errors from retryableErrors. Guard
helmCtx.Log before calling Infof or Warningf.
Comment on lines
+137
to
+232
| if helmCtx.Verbose { | ||
| helmCtx.Log.Dimf("resolved Helm chart for release %s as %q", opts.ReleaseName, chartPath) | ||
| helmCtx.Log.Dimf("values for Helm chart for release %s:", opts.ReleaseName) | ||
| helpers.LogMultilineYaml(helmCtx.Log, opts.Values) | ||
| } | ||
|
|
||
| if helmCtx.DryRun { | ||
| helmCtx.Log.Infof("skipping action for add-on Helm chart %s", opts.ReleaseName) | ||
| return nil | ||
| } | ||
|
|
||
| if helmCtx.Verbose { | ||
| if opts.ChartPath != "" { | ||
| helmCtx.Log.Dimf("installing Helm chart from directory %q as release %s into namespace %s", | ||
| opts.ChartPath, opts.ReleaseName, opts.Namespace) | ||
| } else { | ||
| helmCtx.Log.Dimf("installing Helm chart %s/%s:%s as release %s into namespace %s", | ||
| opts.RepoURL, opts.ChartName, opts.ChartVersion, opts.ReleaseName, opts.Namespace) | ||
| } | ||
| } | ||
|
|
||
| status := releaseStatus(cfg, opts.ReleaseName) | ||
| if status.IsPending() || status == release.StatusFailed { | ||
| helmCtx.Log.Warningf("Helm release %s is in state %q, forcing uninstall before reinstall", opts.ReleaseName, status) | ||
| uninstall := action.NewUninstall(cfg) | ||
| if _, err := uninstall.Run(opts.ReleaseName); err != nil && !strings.Contains(strings.ToLower(err.Error()), "not found") { | ||
| return fmt.Errorf("cleaning up stuck release %s: %w", opts.ReleaseName, err) | ||
| } | ||
| } else if status == release.StatusDeployed { | ||
| return doUpgrade(helmCtx, cfg, opts) | ||
| } | ||
|
|
||
| install := action.NewInstall(cfg) | ||
| install.ReleaseName = opts.ReleaseName | ||
| install.Namespace = opts.Namespace | ||
| install.Wait = false | ||
|
|
||
| chart, err := loader.Load(chartPath) | ||
| if err != nil { | ||
| return fmt.Errorf("loading chart from %q: %w", chartPath, err) | ||
| } | ||
|
|
||
| _, err = install.RunWithContext(helmCtx.Ctx, chart, opts.Values) | ||
| return err | ||
| } | ||
|
|
||
| func doUpgrade(helmCtx HelmCtx, cfg *action.Configuration, opts InstallOptions) error { | ||
| chartPath, err := resolveChart(opts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if helmCtx.Verbose { | ||
| helmCtx.Log.Dimf("resolved Helm chart for release %s as %q", opts.ReleaseName, chartPath) | ||
| } | ||
|
|
||
| if helmCtx.DryRun { | ||
| helmCtx.Log.Infof("skipping upgrade action for add-on Helm chart %s", opts.ReleaseName) | ||
| return nil | ||
| } | ||
|
|
||
| if helmCtx.Verbose { | ||
| helmCtx.Log.Dimf("a Helm release named %s already exists in namespace %s, conducting upgrade", | ||
| opts.ReleaseName, opts.Namespace) | ||
| } | ||
| upgrade := action.NewUpgrade(cfg) | ||
| upgrade.Namespace = opts.Namespace | ||
| upgrade.Wait = false | ||
|
|
||
| chart, err := loader.Load(chartPath) | ||
| if err != nil { | ||
| return fmt.Errorf("loading chart from %q: %w", chartPath, err) | ||
| } | ||
|
|
||
| if helmCtx.Verbose { | ||
| helmCtx.Log.Dimf("values for Helm chart for release %s:", opts.ReleaseName) | ||
| helpers.LogMultilineYaml(helmCtx.Log, opts.Values) | ||
| } | ||
|
|
||
| _, err = upgrade.RunWithContext(helmCtx.Ctx, opts.ReleaseName, chart, opts.Values) | ||
| return err | ||
| } | ||
|
|
||
| func doUninstall(helmCtx HelmCtx, releaseName, namespace string) error { | ||
| if helmCtx.DryRun { | ||
| helmCtx.Log.Infof("skipping uninstall action for add-on Helm chart %s", releaseName) | ||
| return nil | ||
| } | ||
|
|
||
| cfg, err := newActionConfig(helmCtx, namespace) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if helmCtx.Verbose { | ||
| helmCtx.Log.Dimf("uninstalling Helm release %s from namespace %s", releaseName, namespace) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Nil-logger panic risk persists across doInstall/doUpgrade/doUninstall/BuildDependencies.
newActionConfig (lines 279-283) correctly guards with helmCtx.Log != nil, proving Log is expected to be nilable, yet nearly every other log call in this file dereferences it unguarded: lines 137-141, 144, 150-156, 160, 190-191, 194, 199-201, 212-214, 222, 232 (all helmCtx.Log.*), and line 317 (log.Infof in BuildDependencies). Any of these will panic if Log is nil.
Also applies to: 317-317
🤖 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 `@internal/helm/helm.go` around lines 137 - 232, Guard every logging call in
doInstall, doUpgrade, doUninstall, and BuildDependencies before dereferencing
helmCtx.Log or log. Reuse the nil-safe pattern already established in
newActionConfig, ensuring normal logging remains unchanged when a logger is
provided and nil logger contexts complete without panicking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes