Skip to content
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

Allow query parameters on endpoints. Fix various endpoints. #172

Merged
merged 6 commits into from
Apr 14, 2024

Conversation

chintal
Copy link
Collaborator

@chintal chintal commented Apr 11, 2024

Description

Fixes #171
Fixes #173
Fixes #174

Adds a util to format list-type params. Should not break any existing user code, but gives the option to provide such parameter values as a list instead of needing to construct a string of comma separated values.

Checklist

  • The patch has appropriate test coverage
  • The patch follows the style guidelines of this project
  • The patch has appropriate comments, particularly in hard-to-understand areas
  • The documentation was updated corresponding to the patch
  • I have performed a self-review of this patch

@chintal chintal requested a review from amotl as a code owner April 11, 2024 09:20
@amotl
Copy link
Contributor

amotl commented Apr 11, 2024

Dear @chintal,

thank you very much for your contribution. Your patch looks sane, I don't know why the PR fails on CI/GHA, we will need to look into that.

With kind regards,
Andreas.

@chintal
Copy link
Collaborator Author

chintal commented Apr 11, 2024

The failures look like they are some old tests failing. main by itself fails a number of tests. The PR is simple enough that it can be trivially rebased and applied once main is fixed.

@chintal chintal changed the title Implement teams read endpoints for user and actual user. Allow query parameters on endpoints. Fix get_all_folders and add user teams endpoints Apr 12, 2024
@chintal
Copy link
Collaborator Author

chintal commented Apr 12, 2024

My apologies for the combined PR for unrelated issues. I'll try to remember to create the PR from a branch next time.

@chintal
Copy link
Collaborator Author

chintal commented Apr 12, 2024

By the way, the issue with the tests failing seems to be on line 163 of test_grafana_client.py and the behavior (?) of https://example.org.

https://github.com/panodata/grafana-client/blob/7f5712c7953bde359abcdbbfb783af29d0b59b0f/test/test_grafana_client.py#L163

The actual response received from example.com seems to be 500, though the text in the body of the response reads like a 404. So while the test checks for GrafanaClientError, it actually raises GrafanaServerError.

https://github.com/panodata/grafana-client/blob/7f5712c7953bde359abcdbbfb783af29d0b59b0f/grafana_client/client.py#L157-L172

…st_grafana_client_no_verify to not break once params are added in.
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 76.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 92.26%. Comparing base (7303ec2) to head (d341a2b).
Report is 25 commits behind head on main.

Files Patch % Lines
grafana_client/elements/user.py 33.33% 4 Missing ⚠️
grafana_client/elements/search.py 85.71% 2 Missing ⚠️
grafana_client/util.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
- Coverage   94.44%   92.26%   -2.19%     
==========================================
  Files          26       27       +1     
  Lines        1711     1810      +99     
==========================================
+ Hits         1616     1670      +54     
- Misses         95      140      +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines -163 to +165
self.assertRaises(GrafanaClientError, lambda: grafana.connect())
self.assertRaises((GrafanaClientError, GrafanaServerError), lambda: grafana.connect())
Copy link
Contributor

@amotl amotl Apr 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the issue with the tests failing seems to be on line 163 of test_grafana_client.py and the behavior (?) of https://example.org/.

Indeed, example.org, on its /api endpoint, seems to respond with both 500 and 404 status codes now, specifically with HTTP 500 on /api/health.

HTTP/1.1 404 Not Found

http HEAD https://example.org/api/foo
http HEAD https://example.org/api/hello

HTTP/1.1 500 Internal Server Error

http HEAD https://example.org/api/bar
http HEAD https://example.org/api/world
http HEAD https://example.org/api/health

It's weird, but c'est la vie, right?

…tally add util to accept and format list-type params.
@chintal chintal changed the title Allow query parameters on endpoints. Fix get_all_folders and add user teams endpoints Allow query parameters on endpoints. Fix various endpoints. Apr 14, 2024
@amotl
Copy link
Contributor

amotl commented Apr 14, 2024

Dear Chintalagiri,

thank you so much for fixing those embarrassing flaws of grafana-client, and for adding new functionality around the User/Teams API endpoint wrapper. Excellent!

Keep up that good work, and with kind regards,
Andreas.

@@ -42,6 +42,7 @@
AsyncDatasource,
AsyncFolder,
AsyncHealth,
AsyncLibraryElement,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was missing, right? Thanks for spotting, and fixing per 4144e92!

Maybe we can come up with an idea and routine to prevent such details slipping through on future occasions, on behalf of script/generate_async.py, if it's not too much of a hassle?

/cc @Ousret

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be easy to implement, yes.
There's another option, a bit more straightforward (without generate_async.py): "getattr + module async inspect" on init to check if one api is missing in both sync and async. then raise a warning or error.

regards,

@amotl amotl merged commit 690ae38 into grafana-toolbox:main Apr 14, 2024
6 checks passed
@amotl
Copy link
Contributor

amotl commented Apr 14, 2024

grafana-client 4.1.0 has been released, including your fixes and improvements. Thanks again.

NB: Fixing those flaws so diligently and swiftly clearly makes you eligible to be added as a collaborator on this project, so we've just invited you. Feel free to join if that fits your bill.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants