docs(weather-agent): fix installer secret namespace in demo-ui#439
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR updates the weather-agent demo UI docs to (1) list only installer-provided ConfigMaps for ChangesDocumentation Clarification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@authbridge/demos/weather-agent/demo-ui.md`:
- Around line 110-115: The troubleshooting step currently instructs users to
check the secret `keycloak-admin-secret` in `team1` which contradicts the
earlier note that Operator 0.2+ stores it in `kagenti-system`; update the
command and any related text that references `team1` so it checks
`kagenti-system` instead (e.g., replace `kubectl get secret
keycloak-admin-secret -n team1` with `kubectl get secret keycloak-admin-secret
-n kagenti-system`) and clarify that `NotFound` in `team1` is expected when
Operator 0.2+ is used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1f781da-473d-4ab6-a1c4-d9b5800fa6a1
📒 Files selected for processing (1)
authbridge/demos/weather-agent/demo-ui.md
Clarify keycloak-admin-secret lives in kagenti-system (not team1) on operator 0.2+, and point UI login to kagenti-test-user in keycloak. Fixes kagenti#1679 Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
8935a78 to
774c94f
Compare
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Small, well-targeted docs fix — the new "Installer-Provided Resources" block is clearer and more accurate. One blocker before merge plus an out-of-scope nit.
must-fix
authbridge/demos/weather-agent/demo-ui.md:569 — this troubleshooting step still checks team1 for keycloak-admin-secret:
kubectl get secret keycloak-admin-secret -n team1That directly contradicts the new note added on line 110 of this same PR. Mariusz, you fixed it up top then walked right past it down here — must be a Monday 😄. Please flip this to -n kagenti-system so the doc is consistent end-to-end. CodeRabbit flagged this one too.
suggestion (out of scope — feel free to skip)
authbridge/demos/weather-agent/demo-ui.md:562-563 — "the client-registration sidecar couldn't register the client" is stale terminology. Per authbridge/CLAUDE.md, the in-pod client-registration sidecar has been removed and the kagenti-operator's ClientRegistrationReconciler handles registration now. Could read something like "the operator's client registration couldn't reach Keycloak" instead. Happy to defer this to a follow-up.
Areas reviewed: Docs (single markdown file, ±25 lines)
Commits: signed-off ✅
CI status: all green ✅
Address review feedback on rossoctl#439: - Troubleshooting step now checks keycloak-admin-secret in kagenti-system, matching the note added earlier in the same doc (operator 0.2+). - Update stale "client-registration sidecar" wording to reference the operator's ClientRegistrationReconciler, which handles client registration in operator 0.2+. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
|
@huang195 both items addressed in a11c09a:
Re Monday — guilty as charged 😄 |
The Step 3 verification claimed 2/2 with a single combined AuthBridge sidecar, which is only true for proxy-sidecar mode without SPIRE. Real installs commonly run with `kagenti.io/spire: enabled` and/or envoy-sidecar mode and see 3/3 (`agent envoy-proxy spiffe-helper`). State the expected output explicitly for the cluster-default mode and note that the count depends on AuthBridge mode + SPIRE setting. Link to the deployment guide for the full mode/label reference. Tracking the broader docs gap in rossoctl/rossoctl#1775. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/demos/weather-agent/demo-ui.md`:
- Around line 230-245: The expected output in Step 3 currently shows a `2/2`
container count which contradicts Step 2's instruction to keep SPIRE identity
enabled by default; update the Step 3 example to reflect the SPIRE-enabled
default (show `3/3` for the weather-service when spiffe-helper is present) or
explicitly state that the snippet assumes SPIRE was manually disabled in Step 2,
and adjust the header/note text so the example and the note about modes
(`proxy-sidecar`/`envoy-sidecar` and `spiffe-helper`) are consistent with the
Step 2 guidance.
- Around line 232-236: The fenced code block showing the pod table ("NAME READY
STATUS RESTARTS AGE ... weather-tool-7f8c9d6b44-yyyyy ...") lacks a language
tag; update that Markdown fenced block to include a language specifier (e.g.,
change "```" to "```text" or "```bash") so it satisfies markdownlint MD040 and
renders correctly (locate the fenced block containing the NAME/READY/STATUS
lines in demo-ui.md and add the language tag).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b8648fe-3b58-418c-b97c-36db06bc2a25
📒 Files selected for processing (1)
authbridge/demos/weather-agent/demo-ui.md
…efault Step 2 instructs users to keep "Enable SPIRE identity" checked by default, but Step 3's expected pod output assumed SPIRE was disabled (2/2). A user following the defaults sees 3/3 and may believe the deployment is wrong. Make the Step 2-default case (proxy-sidecar + SPIRE = 3/3) the primary example, and document the no-SPIRE 2/2 case as the alternate. Apply the same alignment to the "Verify injected containers" output. Tag fenced code blocks as 'text' to satisfy markdownlint MD040. Addresses CodeRabbit review comments on demo-ui.md:236 and :245. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Address review feedback that the previous structure was too verbose and too kubectl-heavy for a "UI demo." Rewrite the file end-to-end so the primary flow runs in the Kagenti UI, mirroring the github-issue UI demo's pattern: - Open with a "How this differs from the standard demo" comparison table so users know up-front what changes vs. demo-ui.md. - Step 1: one Keycloak script invocation. - Step 2: import the tool via the UI with AuthBridge + SPIRE checked. - Step 3: import the agent via the UI; LLM env loaded via Import from URL using the existing beginner .env.openai / .env.ollama files, with one MCP_URL edit to point at the advanced tool. Outbound Routing Rules expander replaces the manual configmaps-advanced.yaml apply for backends that support it (kagenti#1194); fallback to kubectl apply called out as a one-liner. - Step 4: chat in the UI. - Step 5 (optional): deploy_and_verify_advanced.sh for CLI / CI users. Move the existing kubectl + manifest flow + AgentRuntime gotchas to a clearly-marked "Appendix: kubectl-only path" so users who prefer the manifest path still have it, without it dominating the doc. Mirror PR rossoctl#439's "keycloak-admin-secret is not in team1; operator 0.2+ keeps it in kagenti-system" phrasing in Prerequisites for consistency across the two weather demos. Keep the LLM troubleshooting rows from the previous commit. The underlying gap (deploy_and_verify_advanced.sh does not exercise the LLM path) still exists and remains the demo's most common surprise. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Docs-only fix (single markdown file, +34/−28 lines) that corrects namespace references for keycloak-admin-secret to reflect operator 0.2+ behavior, aligns container count expectations with SPIRE-enabled defaults, and updates stale sidecar terminology. All prior review feedback from @huang195 and CodeRabbit has been addressed across 3 follow-up commits.
Areas reviewed: Docs
Commits: 4 commits, all signed-off ✅
CI status: all green ✅
|
@huang195 — friendly ping 🙏 Both of your concerns from the May-27 review are addressed on HEAD:
@pdettori has approved as of today, and CI is green. Could you dismiss / re-review when you have a sec? It would unblock the merge. 🙏 |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Thanks Mariusz — the namespace fixes (kagenti-system) and the ClientRegistrationReconciler terminology are spot on now, and the installer-resources cleanup reads great. One blocker though: the new 3/3 container model (separate spiffe-helper) describes the pre-#411 / v0.6 layout, not main. On main, spiffe-helper is bundled inside the combined sidecar and the weather-service pod is 2/2 whether or not SPIRE is enabled — your own linked deployment-guide.md confirms "1 combined sidecar." The 2/2 text this PR removes was actually correct. Revert the container-count changes (lines 230-257) to the combined-sidecar description and this is good to go.
Areas reviewed: Docs · Commits: signed-off ✅ · CI: all green ✅
Assisted-By: Claude Code
| ```text | ||
| NAME READY STATUS RESTARTS AGE | ||
| weather-service-58768bdb67-xxxxx 2/2 Running 0 2m | ||
| weather-service-58768bdb67-xxxxx 3/3 Running 0 2m |
There was a problem hiding this comment.
Mariusz, mamma mia 😄 — this flips the pod count to 3/3, but on kagenti main spiffe-helper is bundled inside the combined sidecar (gated by SPIRE_ENABLED), it's not its own container. The pod is 2/2 here regardless of SPIRE. The 3/3 separate-spiffe-helper layout is the old pre-#411 / v0.6 model. The line you deleted ("2/2 … combined sidecar, spiffe-helper bundled inside") was right for main — let's keep that.
Refs: charts/kagenti/values.yaml:299-301, docs/authbridge/deployment-guide.md:10.
There was a problem hiding this comment.
You are right — fixed in 189bcdb. Reverted to 2/2 and rewrote the note to match the combined-sidecar architecture (spiffe-helper bundled inside, gated by SPIRE_ENABLED). My earlier 8dacc17 was describing the pre-#411 / v0.6 layout, which I confused with the current main. Thanks for the patient explanation. 🍝
| > **Note:** The container count depends on the AuthBridge mode and whether | ||
| > SPIRE identity is enabled. With Step 2's defaults — `proxy-sidecar` mode | ||
| > plus SPIRE identity — `weather-service` runs `agent` + `authbridge-proxy` | ||
| > + `spiffe-helper` (`3/3`). If you unchecked **Enable SPIRE identity**, the |
There was a problem hiding this comment.
Same root cause — there's no + spiffe-helper (3/3) vs 2/2-without-SPIRE split on main. SPIRE on/off doesn't change the container count; it just toggles spiffe-helper inside the combined sidecar. And the guide you link two lines down literally says "1 combined sidecar" — so this note contradicts its own reference. Dobra robota otherwise, just need this aligned. 😉
There was a problem hiding this comment.
Fixed in 189bcdb. New note now says explicitly "2/2 regardless of whether SPIRE identity is enabled" and credits the combined-sidecar architecture rather than fighting it. No more contradiction with the deployment guide.
| Expected (Step 2 defaults — `proxy-sidecar` mode with SPIRE identity): | ||
|
|
||
| ```text | ||
| agent authbridge-proxy spiffe-helper |
There was a problem hiding this comment.
agent authbridge-proxy spiffe-helper won't match on main — kubectl ... containers[*].name returns just agent authbridge-proxy (2 names) in proxy mode, SPIRE or not. Drop the spiffe-helper name and the "without SPIRE" variant collapses into the same output.
There was a problem hiding this comment.
Fixed in 189bcdb. Dropped spiffe-helper from the expected containers[*].name output and removed the "without SPIRE" variant — it now shows agent authbridge-proxy (or agent envoy-proxy in envoy-sidecar mode) with a one-line note that the names do not change with SPIRE.
Hai is right — on kagenti main, AuthBridge ships as a single combined sidecar image (kagenti-extensions#411). The weather-service pod is 2/2 regardless of SPIRE: spiffe-helper is bundled inside the combined image and activated per workload via SPIRE_ENABLED, not as a separate container. The deployment guide explicitly lists "1 combined sidecar" for both proxy-sidecar and envoy-sidecar modes (docs/authbridge/deployment-guide.md and charts/kagenti/values.yaml). The 3/3 separate-spiffe-helper layout I introduced in 8dacc17 described the pre-rossoctl#411 / v0.6 model — wrong for main. Revert the container-count claim to 2/2 in both "Check pod status" and "Verify injected containers" sections, and update the explanatory note to match the combined-sidecar architecture. Container names returned by kubectl ... containers[*].name are 'agent authbridge-proxy' (proxy mode) or 'agent envoy-proxy' (envoy mode) regardless of SPIRE. Addresses huang195's review on PR rossoctl#439. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
|
@huang195 fixed in 189bcdb — you were right, my 8dacc17 was describing the pre-#411 / v0.6 separate-spiffe-helper layout, not current |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Mariusz nailed it 💪 — the Step 3 container model now correctly reflects main: a single combined AuthBridge sidecar (2/2) regardless of SPIRE, with spiffe-helper bundled inside and gated by SPIRE_ENABLED (via the kagenti.io/spire: enabled label), and envoy-sidecar still 2/2 + a proxy-init init container. That matches docs/authbridge/deployment-guide.md exactly. Combined with the earlier kagenti-system namespace and ClientRegistrationReconciler fixes, every claim in this doc is now accurate for main. Dziękuję for the quick turnaround! Clearing my earlier change request.
Areas reviewed: Docs · Commits: 5, all signed-off ✅ · CI: all green ✅
Assisted-By: Claude Code
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Kagenti Issue Prioritization
4 participants
Summary
Corrects Installer-Provided Resources in the weather beginner demo:
keycloak-admin-secretis inkagenti-system, notteam1(operator 0.2+)kagenti-test-userinkeycloakkeycloak-admin-secretinteam1Net shorter section (−5 lines).
Issue
Part of rossoctl/rossoctl#1679
Test plan
kubectl get secret keycloak-admin-secret -n kagenti-systemon Kind installkubectl get secret keycloak-admin-secret -n team1→ NotFound (expected)Summary by CodeRabbit