Support deploying add-on Helm charts alongside central#242
Closed
mclasmeier wants to merge 18 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 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, teardown configuration restoration, integration tests, E2E coverage, and documentation. ChangesHelm chart add-on lifecycle
Estimated code review effort: 4 (Complex) | ~60 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}
Contributor
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
July 15, 2026 22:50
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
July 16, 2026 12:25
6575e1b to
171705f
Compare
Contributor
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
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
July 16, 2026 20:19
171705f to
e8a0dda
Compare
Contributor
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
mclasmeier
force-pushed
the
mc/helm-chart-add-ons
branch
from
July 16, 2026 20:29
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. |
Contributor
🧩 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. |
Collaborator
Author
|
@coderabbitai, check if I have addressed everything. Resolve accordingly. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="stackrox/roxie"
pr="242"
echo "== Current PR revision =="
gh pr view "$pr" --repo "$repo" --json headRefOid,updatedAt,url --jq '{url,headRefOid,updatedAt}'
echo "== Unresolved CodeRabbit threads (full text) =="
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 createdAt }
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false and any(.comments.nodes[]; .author.login == "coderabbitai"))
| {id,path,line,originalLine,comments:[.comments.nodes[] | {author:.author.login,body,url,createdAt}]}'
echo "== Relevant implementation outlines =="
for f in internal/helm/helm.go internal/deployer/addons.go cmd/teardown.go; do
echo "--- $f ---"
ast-grep outline "$f" --items all
done
echo "== Current relevant source =="
for spec in \
"internal/helm/helm.go:1,380" \
"internal/deployer/addons.go:1,220" \
"cmd/teardown.go:1,150"; do
file="${spec%%:*}"
range="${spec#*:}"
start="${range%,*}"
end="${range#*,}"
echo "--- $file, Lines $start-$end ---"
nl -ba "$file" | sed -n "${start},${end}p"
done
echo "== Relevant tests and current PR delta =="
for f in internal/deployer/addons_test.go internal/helm/helm_integration_test.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f"
fi
done
git diff --find-renames --unified=30 "$(git merge-base HEAD origin/main)"...HEAD -- \
internal/helm/helm.go internal/deployer/addons.go cmd/teardown.go internal/deployer/addons_test.goLength of output: 10947 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== internal/helm/helm.go (current) =="
cat -n internal/helm/helm.go
echo "== Retry helper and logging call sites =="
rg -n -C 4 \
'\b(executeHelmActionWithRetries|isRetryable|doInstall|doUpgrade|doUninstall|BuildDependencies)\b|helmCtx\.Log\.|[^[:alnum:]_]log\.(Infof|Warningf|Errorf|Debugf)\s*\(' \
internal/helm/helm.go
echo "== Helm unit/integration test candidates =="
fd -t f -e go . internal/helm | sort
for f in $(fd -t f -e go . internal/helm | sort); do
echo "--- $f ---"
rg -n -C 3 \
'\b(executeHelmActionWithRetries|isRetryable|BuildDependencies|HelmCtx|Install|Uninstall)\b' \
"$f" || true
done
echo "== Current relevant PR diff =="
git diff --find-renames --unified=40 "$(git merge-base HEAD origin/main)"...HEAD -- internal/helm/helm.goLength of output: 34569 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Current add-on lifecycle implementation =="
cat -n internal/deployer/addons.go
echo "== Add-on deployment/teardown call sites and tests =="
rg -n -C 5 \
'\b(deployAddOns|teardownAddOns|Deploy|Teardown|IsOptional|IncludesAddOns)\s*\(' \
internal/deployer tests --glob '*.go'
echo "== Add-on-related tests =="
for f in $(fd -t f -e go . internal/deployer tests | rg '(addons|deployer).*_test\.go$'); do
echo "--- $f ---"
cat -n "$f"
done
echo "== Relevant current PR diff =="
git diff --find-renames --unified=35 "$(git merge-base HEAD origin/main)"...HEAD -- \
internal/deployer/addons.go internal/deployer/deployer.go internal/deployer/addons_test.goLength of output: 69668
No newly addressed CodeRabbit comments were available to resolve. |
Collaborator
Author
|
@coderabbitai, regarding the danger of nil loggers, doesn't commit 0c80659 take care of that? Regarding
I don't see how that could happen. The string "context deadline exceeded" doesn't contain the word "timeout", how would that match? And even if it would(!), on a retry we would immediately trigger the early return in line 110. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Specified commit =="
git show --format=fuller --stat 0c8065940d39ada66160bd54f424bcdd1d8a6c95
git show --format= --unified=20 0c8065940d39ada66160bd54f424bcdd1d8a6c95 -- internal/logger/logger.go internal/helm/helm.go
echo "== Logger method outline and implementation =="
ast-grep outline internal/logger/logger.go --items all
nl -ba internal/logger/logger.go | sed -n '1,260p'
echo "== Helm retry helper and retryable-error matching =="
ast-grep outline internal/helm/helm.go --items all
nl -ba internal/helm/helm.go | sed -n '70,145p'
rg -n -C 5 'retry|Retry|timeout|deadline|context\.|isRetryable|executeHelmActionWithRetries' \
internal/helm --glob '*.go'
echo "== Existing unresolved CodeRabbit threads on Helm =="
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 line originalLine
comments(first: 20) { nodes { author { login } body url } }
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false and .path == "internal/helm/helm.go")
| {id,path,line,originalLine,comments:[.comments.nodes[] | {author:.author.login,body,url}]}'Length of output: 5987 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Helm retry helper and retryable-error matching =="
cat -n internal/helm/helm.go | sed -n '70,145p'
rg -n -C 6 'retry|Retry|timeout|deadline|context\.|isRetryable|executeHelmActionWithRetries' \
internal/helm --glob '*.go'
echo "== Unresolved CodeRabbit Helm 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 line originalLine
comments(first: 20) { nodes { author { login } body url } }
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false and .path == "internal/helm/helm.go")
| {id,path,line,originalLine,comments:[.comments.nodes[] | {author:.author.login,body,url}]}'Length of output: 15121 🏁 Script executed: #!/bin/bash
set -euo pipefail
for thread_id in \
"PRRT_kwDOPn6LOs6RcLgl" \
"PRRT_kwDOPn6LOs6RcLgp"; 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)"'
doneLength of output: 589
Thank you for the correction. 🐇 |
Collaborator
Author
That's by design, for this exact reason we have the |
Contributor
|
My best-effort-only assumption was incorrect, so I’ve withdrawn that concern and resolved the remaining review comments. ✏️ Learnings added
✅ Action performedComments resolved. Approval is disabled; enable |
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
… secret: failed to create admin password secret: kubectl command failed
mclasmeier
force-pushed
the
mc/helm-chart-add-ons
branch
from
July 19, 2026 14:02
a0af7fa to
a27ae7d
Compare
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-onSummary by CodeRabbit
New Features
roxie-addon-prefix, with priority-based ordering.addons/add-onscomponent selection.Bug Fixes
Documentation
Tests