Repository navigation
docs(monitoring): correct the release name the OIDC comment, guide and test fixture encode - #3468
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
area/monitoring
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The documented platform and tenant Grafana naming matches the checked-in deployment path. No actionable merge-blocking risk is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review details
🚥 Pre-merge checks | ✅ 5
✨ Finishing Touches
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4da6df5906
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
|
||
| Cozystack ships one Grafana codebase deployed twice: as | ||
| `monitoring-system` in the platform's `cozy-monitoring` namespace and as | ||
| `monitoring-system` in the platform's `tenant-root` namespace and as |
There was a problem hiding this comment.
The edited paragraph remains split across lines 3–8, but this repository requires Markdown prose paragraphs to use one continuous line; join the paragraph rather than changing only one wrapped line.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
|
|
||
| Cozystack ships one Grafana codebase deployed twice: as | ||
| `monitoring-system` in the platform's `cozy-monitoring` namespace and as | ||
| `monitoring-system` in the platform's `tenant-root` namespace and as |
There was a problem hiding this comment.
For every non-root tenant, the next line still claims that Grafana is deployed as monitoring. However, packages/extra/monitoring/templates/helmrelease.yaml:4 names the inner release {{ .Release.Name }}-system, and the built-in outer release is monitoring, so the Grafana-rendering release is monitoring-system, as the updated helper now states. Leaving this unchanged preserves the exact release-name error this commit intends to correct and gives operators the wrong OIDC resource-name prefix.
Useful? React with 👍 / 👎.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM, one blocker, and it is a one-line fix.
The diff corrects line 4 of the opening paragraph but leaves line 5 saying the tenant instance is released as monitoring. That is the same error this PR exists to remove: the per-tenant Grafana is monitoring-system as well, and only the namespace differs between the two instances. The chain is packages/apps/tenant/templates/monitoring.yaml creating HR monitoring in the tenant namespace, then packages/extra/monitoring/templates/helmrelease.yaml creating the inner HR {{ .Release.Name }}-system which carries system/monitoring and sets neither releaseName nor targetNamespace, so Flux defaults the release name to monitoring-system. The root tenant goes through that same path, which is exactly why the platform and tenant instances differ by namespace only. After this commit the guide and the helper comment contradict each other on that point.
An operator following line 5 would compute <ns>-monitoring, <ns>-monitoring-audience, monitoring-oidc-client and monitoring-oidc-users, all four wrong.
Everything else holds up. I re-derived the chain independently, and the eight OIDC-derived names in the fixture plus both entries in the helper comment are correct. I also checked that the rename does not make the fixture vacuous: patching the clientId helper to drop the namespace still fails two tests, and the nested-tenant case keeps namespace and release name as distinct inputs. helm unittest packages/system/monitoring passes on your head commit, 6 suites and 47 tests.
While rewriting that sentence, please join the paragraph into a single line. Markdown prose here is one line per paragraph, and the fix rewrites both wrapped lines anyway.
Non-blocking, and not something I expect this PR to absorb: five of the seven fixtures under packages/system/monitoring/tests/ still declare release.name: monitoring, and only storage_pvc_label_test.yaml uses monitoring-system. Nothing renders wrong today because no assertion in the other files depends on the release name, but it is the same drift a few files over and worth a follow-up.
Thanks for picking this up, it needed doing.
| Cozystack ships one Grafana codebase deployed twice: as | ||
| `monitoring-system` in the platform's `cozy-monitoring` namespace and as | ||
| `monitoring-system` in the platform's `tenant-root` namespace and as | ||
| `monitoring` in every tenant namespace. Both instances opt in to OIDC |
There was a problem hiding this comment.
This is the other half of the same error. The per-tenant release is monitoring-system too, so as written this now contradicts the _helpers.tpl comment that the same commit fixes.
|
Thanks - you're right, I was too conservative on that line. Fixed in f7cec70: both instances are now described as On the non-blocking note: agreed - the other five fixtures under |
|
This was addressed in f7cec70 (pushed after your review): the opening paragraph is now a single line and both instances read |
f7cec70 to
140e718
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
KBS (@youdie006) NOT LGTM. The docs fix itself is right now. What is left is text that would land in main with it, plus a rebase.
The blocker from my last review is fixed: the guide's opening paragraph describes both instances as monitoring-system and is one line. On a trial merge with current main the monitoring chart tests pass, 16 suites and 119 tests.
Three things before this can merge, none of them code:
- The PR body becomes the merge commit message, and two parts of it are no longer true. The
docs/oidc-grafana.mdbullet says the release name there "was already correct, so only the namespace is changed", but the diff now also changes the tenant instance frommonitoringtomonitoring-system. The CodeRabbit summary at the end lists "New Features" and says resource identifiers were renamed, which contradicts "Nothing changes at runtime" above it. Please fix the bullet and delete the generated block. The bot adds it back on every push, so edit the body after you push. Also, the OIDC suite has 39 tests on current main, so "37/37" will be stale after the rebase. - The body says the change was prepared with AI assistance, but commit 140e718 has no
Assisted-by: LLMtrailer. The contributing guide asks for exactly one on a commit an AI tool helped with. - Please rebase onto current main when you amend. The
E2E Testsand pre-commit runs on this head are from 2026-08-29, when CI was broken for every fork PR (fixed on 2026-09-27). This change has never had a real CI run.
140e718 to
cd392d2
Compare
|
Rebased onto current main and added the |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
KBS (@youdie006) NOT LGTM, and the trailer is the only reason. Commit cd392d2 carries Assisted-By: Claude <[email protected]>, and the contributing guide lists exactly this line as a blocker: the value has to be LLM, with no model or vendor name. Please amend it to Assisted-by: LLM and force-push. The Commit trailers CI job fails on it as well, it just has not run yet because the fork runs are waiting for approval.
Everything else from my last review is done. The patch is the same one I reviewed, now rebased. On a trial merge with current main the monitoring chart tests pass, 23 suites and 301 tests.
cd392d2 to
cfb61db
Compare
…d test fixture encode The naming block in _helpers.tpl, the guide's opening paragraph and the oidc test fixture all describe the per-tenant Grafana as the release `monitoring`. Both instances are released as `monitoring-system` by the monitoring application and differ only by namespace: the platform one in `tenant-root`, a per-tenant one in each tenant namespace. Assisted-by: LLM Signed-off-by: youdie006 <[email protected]>
cfb61db to
b7708fb
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
KBS (@youdie006) LGTM, thanks. The trailer is fixed and nothing else changed. I rebased the branch onto current main so CI runs on a fresh base.
2 participants
What this PR does
The Grafana OIDC
clientId, audience-scope name, client-secret name and users-Job name are all derived from the Monitoring release name. In a running cluster that release is alwaysmonitoring-system: the tenant chart's outer HelmReleasemonitoringpoints atextra/monitoring, whosetemplates/helmrelease.yamlcreates the inner HelmRelease{{ .Release.Name }}-systemthat carriessystem/monitoringand renders OIDC. There is no release namedmonitoringthat renders OIDC.Three places encoded a release named
monitoring(i.e.<ns>-monitoring, where a cluster renders<ns>-monitoring-system), per #3338:packages/system/monitoring/templates/_helpers.tplheader comment — both example lines were wrong. The per-tenant release ismonitoring-system, and the platform Grafana lives in thetenant-rootnamespace, notcozy-monitoring. (cozy-monitoringonly hosts the ExternalName services that redirect to the platform stack the root tenant hosts intenant-root— seepackages/system/cozystack-basics/templates/monitoring-external-services.yaml.)docs/oidc-grafana.md— placed the platform release incozy-monitoring; it lives intenant-root. It also named the per-tenant releasemonitoring; that ismonitoring-systemtoo, so both instances now readmonitoring-systemand differ only by namespace.packages/system/monitoring/tests/oidc_test.yaml— the suite pinnedrelease.name: monitoring, so its clientId / audience-scope / client-secret / users-Job assertions pinned names no cluster renders. Both fixtures are set tomonitoring-systemand the derived expected values updated to what a cluster actually renders.Nothing changes at runtime — the templates are self-consistent whatever the release is called; this only aligns the descriptions and the test fixture with reality.
Out of scope here (from the issue, left for a maintainer decision):
cozystack.monitoringPackageSource inpackages/core/platform/sources/monitoring.yaml(issue item 3) — removing a source is a call I can't safely make.Verification, rebased on current main:
helm unittest -f 'tests/oidc_test.yaml' packages/system/monitoring— 39/39 pass; the whole monitoring chart passes as well. After setting the fixtures I updated only the assertions helm-unittest reported as newly diverging.Disclosure: prepared with AI assistance; I reviewed the change and verified the release-name topology against the source files cited above.
Screenshots
N/A — no UI changes.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff, file by file. The three changed paths are asystem/monitoringchart_helpers.tplcomment, the repo-internaldocs/oidc-grafana.md, and a chart test fixture. None match a listed trigger: no change underpackages/apps/orpackages/extra/, nopackages/core/platform/values.yaml, and no variant/bundle/component/release-asset/Talos/tooling change. So no downstream repository is mechanically affected. (The website's own OIDC page referenced in cozystack/website#597 is a separate artifact the reporter is already engaged with, outside this repo's trigger map.)Release note
Summary by CodeRabbit
monitoring-systemrelease name. The platform instance runs intenant-root, while tenant instances run in their respective tenant namespaces.