-
Notifications
You must be signed in to change notification settings - Fork 37
AMG compatibility: Fetch Grafana version from /api/frontend/settings
#229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary by CodeRabbit
WalkthroughThe codebase updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Health
participant GrafanaAPI
Caller->>Health: check()
Health->>GrafanaAPI: GET /api/frontend/settings
GrafanaAPI-->>Health: { buildInfo: {...}, ... }
Health-->>Caller: buildInfo
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
grafana_client/elements/health.py (1)
15-17
: Good consistency with async implementation.The synchronous implementation correctly mirrors the async version changes:
- Same endpoint (
/frontend/settings
)- Same return value extraction (
buildInfo
only)- Same breaking change implications as noted in the async version review
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md
(1 hunks)grafana_client/elements/_async/health.py
(1 hunks)grafana_client/elements/health.py
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
grafana_client/elements/_async/health.py (2)
test/elements/test_health.py (2)
test_healthcheck
(13-21)HealthTestCase
(8-21)test/elements/test_plugin.py (1)
test_health
(211-217)
🔇 Additional comments (3)
CHANGELOG.md (1)
5-6
: LGTM! Clear and accurate changelog entry.The changelog entry properly documents the endpoint change for AMG compatibility and includes appropriate attribution.
grafana_client/elements/_async/health.py (1)
11-11
: LGTM! Updated docstring accurately reflects AMG compatibility.The docstring correctly documents that the method now works with both Grafana and Amazon Managed Grafana.
grafana_client/elements/health.py (1)
11-11
: LGTM! Consistent docstring with async version.The docstring update matches the async implementation and accurately describes AMG compatibility.
50b6e5b
to
c1b887f
Compare
c1b887f
to
0aea049
Compare
0aea049
to
21130a4
Compare
21130a4
to
73847eb
Compare
73847eb
to
1c330d5
Compare
1c330d5
to
881a820
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
==========================================
- Coverage 92.40% 92.26% -0.15%
==========================================
Files 27 27
Lines 1818 1822 +4
==========================================
+ Hits 1680 1681 +1
- Misses 138 141 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
881a820
to
7811c51
Compare
... instead of `/api/health`.
7811c51
to
1585f63
Compare
Problem
Using the
/api/health
endpoint is problematic, because Amazon Managed Grafana (AMG) doesn't support it.Solution
Apply the solution as suggested by @squadgazzz to use the
/api/frontend/settings
endpoint instead -- thanks again!References
/api/frontend/settings
grafana-wtf#144/cc @bhks, @philipnrmn, @ImerysOTDataAndArchitecture, @interfan7