Support deploying add-on Helm charts alongside central#242
Open
mclasmeier wants to merge 12 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
📝 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 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
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>
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
+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.
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>
mclasmeier
force-pushed
the
mc/helm-chart-add-ons
branch
from
171705f to
e8a0dda
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/deployer/addons.go (1)
107-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSort order is nondeterministic for add-ons sharing the same non-zero priority.
The tie-break by name only applies
if a.Priority() == 0 && b.Priority() == 0. For two add-ons with equal non-zero priority,cmp.Comparereturns 0, and combined with the randomized map-iteration order feedingaddOns(line 86) plusslices.SortFunc's non-stable sort, their relative deploy order becomes nondeterministic across runs.🛠️ Proposed fix
slices.SortFunc(addOns, func(a, b AddOn) int { - if a.Priority() == 0 && b.Priority() == 0 { + if a.Priority() == b.Priority() { return cmp.Compare(a.Name(), b.Name()) } return cmp.Compare(b.Priority(), a.Priority()) })🤖 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 107 - 114, Update the comparator passed to slices.SortFunc in the add-on sorting flow so every equal-priority pair, including non-zero priorities, is ordered deterministically by add-on name. Preserve descending priority order and apply the name tie-break only after priorities compare equal.
🤖 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/teardown.go`:
- Around line 58-69: The 30-minute teardown timeout currently includes the
preliminary manifest.LoadManifestSecret operation. Move context.WithTimeout and
its deferred cancel to immediately before the d.Teardown invocation, while using
a non-expiring context for manifest loading and preserving the existing teardown
deadline.
In `@internal/helm/helm.go`:
- Around line 38-50: Remove "context deadline exceeded" from the retryableErrors
list so canceled or deadline-exceeded Helm actions fail fast. Leave the other
transient connection and network errors retryable, preserving RunWithContext
behavior without retrying an operation that may still be running.
---
Nitpick comments:
In `@internal/deployer/addons.go`:
- Around line 107-114: Update the comparator passed to slices.SortFunc in the
add-on sorting flow so every equal-priority pair, including non-zero priorities,
is ordered deterministically by add-on name. Preserve descending priority order
and apply the name tie-break only after priorities compare equal.
🪄 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: 6eceb411-4eb1-4a18-adc2-8f1dd7f1e153
📒 Files selected for processing (15)
cmd/teardown.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/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 (8)
- internal/helm/testdata/minimal-test-chart/Chart.yaml
- internal/deployer/addons_helm_chart.go
- tests/e2e/addons_test.go
- internal/deployer/addons_test.go
- internal/deployer/config_addons.go
- internal/deployer/addons_stackrox_helm_chart.go
- internal/deployer/config.go
- internal/deployer/deployer.go
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
e8a0dda to
a32459c
Compare
Collaborator
Author
|
@coderabbitai, fresh review please. Resolve solved comments, update all your previous comments or delete them if not applicable anymore. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="stackrox/roxie"
pr="242"
echo "== PR metadata =="
gh pr view "$pr" --repo "$repo" --json number,headRefOid,baseRefOid,url,title --jq '{number,title,url,headRefOid,baseRefOid}'
echo "== Changed files =="
gh pr view "$pr" --repo "$repo" --json files --jq '.files[] | "\(.path)\t+\(.additions)/-\(.deletions)"'
echo "== CodeRabbit review comments and thread state =="
gh api graphql \
-F owner="stackrox" \
-F name="roxie" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
originalLine
comments(first: 50) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| {id,isResolved,path,line,originalLine,comments:[.comments.nodes[] | {author:.author.login,body,url}]}'
echo "== Go source outlines for core changed areas =="
for f in \
internal/deployer/addons.go \
internal/deployer/addons_helm_chart.go \
internal/deployer/addons_stackrox_helm_chart.go \
internal/deployer/config_addons.go \
internal/deployer/deployer.go \
internal/helm/helm.go \
cmd/teardown.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
doneLength of output: 54853 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Current core implementations =="
for spec in \
"internal/deployer/deployer.go:304,510" \
"internal/deployer/addons.go:1,170" \
"internal/deployer/addons_helm_chart.go:1,140" \
"internal/component/component.go:1,150" \
"internal/helm/helm.go:1,360" \
"cmd/teardown.go:35,110" \
"internal/deployer/config.go:1,180" \
"internal/deployer/config_addons.go:1,180"; do
file="${spec%%:*}"
ranges="${spec#*:}"
start="${ranges%,*}"
end="${ranges#*,}"
echo "--- $file ($start-$end) ---"
sed -n "${start},${end}p" "$file" | nl -ba -v "$start"
done
echo "== Tests and add-on call sites =="
for f in internal/deployer/addons_test.go internal/helm/helm_integration_test.go tests/e2e/addons_test.go tests/e2e/helpers.go; do
echo "--- $f ---"
cat -n "$f"
done
echo "== Direct callers and component tests =="
rg -n -C 4 \
'\b(Deploy|Teardown|deployAddOns|teardownAddOns|ResolveEnabledAddOns|ListByPrefix|Install|Uninstall)\s*\(' \
--glob '*.go' \
-g '!internal/helm/helm.go' \
-g '!internal/deployer/addons.go' \
-g '!internal/deployer/deployer.go' \
-g '!internal/deployer/addons_helm_chart.go' \
.
echo "== Component-related test files =="
fd -t f -e go . | rg 'component.*_test\.go$|_component.*_test\.go$' || true
echo "== Full merge-base diff for review =="
git diff --find-renames --unified=50 "$(git merge-base HEAD origin/main)"...HEAD -- \
internal/component/component.go \
internal/deployer \
internal/helm \
cmd/teardown.go \
tests/e2e \
README.mdLength of output: 1972 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Current core implementations =="
for spec in \
"internal/deployer/deployer.go:304,510" \
"internal/deployer/addons.go:1,170" \
"internal/deployer/addons_helm_chart.go:1,140" \
"internal/component/component.go:1,150" \
"internal/helm/helm.go:1,360" \
"cmd/teardown.go:35,110" \
"internal/deployer/config.go:1,180" \
"internal/deployer/config_addons.go:1,180"; do
file="${spec%%:*}"
ranges="${spec#*:}"
start="${ranges%,*}"
end="${ranges#*,}"
echo "--- $file ($start-$end) ---"
sed -n "${start},${end}p" "$file" | cat -n
done
echo "== Tests and add-on call sites =="
for f in internal/deployer/addons_test.go internal/helm/helm_integration_test.go tests/e2e/addons_test.go tests/e2e/helpers.go; do
echo "--- $f ---"
cat -n "$f"
done
echo "== Direct callers and component tests =="
rg -n -C 4 \
'\b(Deploy|Teardown|deployAddOns|teardownAddOns|ResolveEnabledAddOns|ListByPrefix|Install|Uninstall)\s*\(' \
--glob '*.go' \
-g '!internal/helm/helm.go' \
-g '!internal/deployer/addons.go' \
-g '!internal/deployer/deployer.go' \
-g '!internal/deployer/addons_helm_chart.go' \
.
echo "== Component-related test files =="
fd -t f -e go . | rg 'component.*_test\.go$|_component.*_test\.go$' || true
echo "== Full merge-base diff for review =="
git diff --find-renames --unified=50 "$(git merge-base HEAD origin/main)"...HEAD -- \
internal/component/component.go \
internal/deployer \
internal/helm \
cmd/teardown.go \
tests/e2e \
README.mdLength of output: 150883 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Manifest persistence and command configuration flow =="
ast-grep outline internal/manifest --items all || true
rg -n -C 5 \
'\b(LoadManifestSecret|CreateManifestSecret|SaveManifest|Manifest|assembleConfigForCommand)\s*\(' \
cmd internal --glob '*.go'
echo "== Manifest and config implementation candidates =="
for f in $(fd -t f -e go . internal cmd | rg '/(manifest|config).*\.go$'); do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo "== Current unresolved CodeRabbit thread IDs =="
gh api graphql \
-F owner="stackrox" -F name="roxie" -F number="242" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id isResolved path originalLine
comments(first: 1) { nodes { author { login } body } }
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false and .comments.nodes[0].author.login == "coderabbitai")
| {id,path,originalLine,body:.comments.nodes[0].body[0:140]}'Length of output: 16616 🏁 Script executed: #!/bin/bash
set -euo pipefail
thread_ids=(
"PRRT_kwDOPn6LOs6RQ-fb"
"PRRT_kwDOPn6LOs6RQ-fg"
"PRRT_kwDOPn6LOs6RQ-fk"
"PRRT_kwDOPn6LOs6RcLgZ"
"PRRT_kwDOPn6LOs6RQ-fp"
"PRRT_kwDOPn6LOs6RQ-ft"
"PRRT_kwDOPn6LOs6RlIS6"
)
for thread_id in "${thread_ids[@]}"; do
gh api graphql \
-F threadId="$thread_id" \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved }
}
}' \
--jq '.data.resolveReviewThread.thread | "\(.id): resolved=\(.isResolved)"'
done
echo "== Remaining unresolved CodeRabbit threads =="
gh api graphql \
-F owner="stackrox" -F name="roxie" -F number="242" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id isResolved path originalLine
comments(first: 1) { nodes { author { login } } }
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false and .comments.nodes[0].author.login == "coderabbitai")
| {id,path,originalLine}'Length of output: 1850
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 45 minutes. |
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.
Add Helm chart add-on support
Add-ons are Helm charts that roxie can deploy alongside ACS. They plug into the existing deploy/teardown lifecycle and are configured via the same config file as the rest of the deployment.
Configuration
Two chart sources are supported:
stackroxRepoHelmChart— chart path relative to the stackrox repo roothelmChart— chart from a public Helm repositoryAdd-ons with higher
prioritydeploy first. Priority-0 add-ons deploy last, sorted by name.Changes
internal/helm— new package wrapping the Helm SDK (install, upgrade, uninstall, status)internal/deployer/addons*.go— add-on resolution, sorting, and deploy/teardown logicinternal/deployer/config_addons.go— config types (CentralAddOnDefinition,CommonAddOnProperties,HelmChartRepoAddOn,StackRoxRepoHelmChartAddOn)internal/component— newAddOnscomponent,IncludesAddOns()/IncludesOperator()methodsinternal/deployer/deployer.go— add-ons wired intoDeploy()andTeardown()lifecyclecmd/teardown.go— teardown now loads the cluster manifest so it knows which add-ons to removeinternal/env— exportedGetStackRoxTopLevelDirfor add-on chart path resolutionTesting
kube-state-metricsas an add-on