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

Add API wrapper for "Plugins" #110

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Add API wrapper for "Plugins" #110

merged 3 commits into from
Sep 15, 2023

Conversation

bhks
Copy link
Contributor

@bhks bhks commented Sep 12, 2023

Description

While working with Grafana I found this amazing python client, I used it to explore the plugin Installation feature using python program. While Install/unInstall APIs are not explicitly called out in Grafana API document. I found it useful to have the base layer on top of which community can build examples and write their own version of how to manage plugins via APIs.

Plugin API link : https://github.com/grafana/grafana/blob/v9.4.x/pkg/api/api.go#L423

Please provide feedback for improvement needed and I should be able to incorporate.

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

@bhks bhks requested a review from amotl as a code owner September 12, 2023 21:36
@amotl
Copy link
Contributor

amotl commented Sep 12, 2023

Dear Bhagwat,

thank you for contributing this excellent patch, I love it, and I think it will be a good candidate to be leveraged by the downstream grafana-wtf package.

May I humbly ask you to take care about code syle / formatting, and add a corresponding software test case 1? You can find relevant instructions within docs/development.md. When running poe format, which is missing to be outlined in the documentation, most of the code style errors will be fixed automatically.

Other than this, if you can afford the time, please also add an item to the list of supported features at README » Overview.

Thank you in advance, and with kind regards,
Andreas.

Footnotes

  1. Saying this, the test suite currently does not have that much value, but at least we have it, and we may migrate it to run real integration tests in the future, see Improve test suite by adding integration tests with Grafana #50.

@amotl amotl changed the title Adding Plugin installation/Uninstalltion Base classes Add API wrapper for Grafana plugins (install/uninstall) Sep 12, 2023
@amotl amotl changed the title Add API wrapper for Grafana plugins (install/uninstall) Add API wrapper for Grafana plugins Sep 12, 2023
@amotl amotl changed the title Add API wrapper for Grafana plugins Add API wrapper for "Plugins" Sep 12, 2023
@bhks
Copy link
Contributor Author

bhks commented Sep 13, 2023

Dear Bhagwat,

thank you for contributing this excellent patch, I love it, and I think it will be a good candidate to be leveraged by the downstream grafana-wtf package.

May I humbly ask you to take care about code syle / formatting, and add a corresponding software test case 1? You can find relevant instructions within docs/development.md. When running poe format, which is missing to be outlined in the documentation, most of the code style errors will be fixed automatically.

Other than this, if you can afford the time, please also add an item to the list of supported features at README » Overview.

Thank you in advance, and with kind regards, Andreas.

Footnotes

  1. Saying this, the test suite currently does not have that much value, but at least we have it, and we may migrate it to run real integration tests in the future, see Improve test suite by adding integration tests with Grafana #50.

Absolutely thanks for calling out the poe format missing. I was trying to just run poe lint and that was giving me error.

Add a corresponding software test case

I think its a good callout, but as you have mentioned we may not have any benefit on the unit test. I have some example code which I believe we can use to add as Integ test. You can find my changes here :main...bhks:grafana-client:main#diff-dc47e20d0c4b3322afa473cbebd824e602265a21bc5b628bb4e2de5496d37a9a

I wanted to first see how my first PR goes and get an opportunity to learn what is the process and how we are thinking about this.

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #110 (e236cb7) into main (5ed1fba) will decrease coverage by 0.50%.
The diff coverage is 78.04%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
- Coverage   96.37%   95.88%   -0.50%     
==========================================
  Files          24       25       +1     
  Lines        1490     1531      +41     
==========================================
+ Hits         1436     1468      +32     
- Misses         54       63       +9     
Files Changed Coverage Δ
grafana_client/elements/plugin.py 76.92% <76.92%> (ø)
grafana_client/api.py 100.00% <100.00%> (ø)
grafana_client/elements/__init__.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bhks
Copy link
Contributor Author

bhks commented Sep 13, 2023

Other than this, if you can afford the time, please also add an item to the list of supported features at README » Overview.

Updated the Readme, Thanks for pointing that out.

@bhks
Copy link
Contributor Author

bhks commented Sep 13, 2023

When running poe format, which is missing to be outlined in the documentation, most of the code style errors will be fixed automatically.

Added to Development document to run poe format

Copy link
Contributor

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thanks for bringing in those improvements. I've added two more nits.

README.md Outdated
Comment on lines 171 to 196
| API | Status |
|---|---|
| Admin | + |
| Alerting | +- |
| API | Status |
|--------------------------------|---|
| Admin | + |
| Alerting | +- |
| Alerting Notification Channels | + |
| Alerting Provisioning | + |
| Annotations | + |
| Authentication | +- |
| Dashboard | + |
| Dashboard Versions | + |
| Dashboard Permissions | + |
| Data Source | + |
| Data Source Permissions | + |
| External Group Sync | + |
| Folder | + |
| Folder Permissions | + |
| Folder/Dashboard Search | +- |
| Health | + |
| Organisation | + |
| Other | + |
| Preferences | + |
| Rbac | +- |
| Snapshot | + |
| Teams | + |
| User | + |
| Alerting Provisioning | + |
| Annotations | + |
| Authentication | +- |
| Dashboard | + |
| Dashboard Versions | + |
| Dashboard Permissions | + |
| Data Source | + |
| Data Source Permissions | + |
| External Group Sync | + |
| Folder | + |
| Folder Permissions | + |
| Folder/Dashboard Search | +- |
| Health | + |
| Organisation | + |
| Other | + |
| Plugin | + |
| Preferences | + |
| Rbac | +- |
| Snapshot | + |
| Teams | + |
| User | + |
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 whole block got reformatted. I know it's a pain, my editor is also playing tricks on me here. Can I ask you to edit only the spot at hand? Apologies for the nit.

docs/development.md Outdated Show resolved Hide resolved
@amotl
Copy link
Contributor

amotl commented Sep 13, 2023

Adding integration tests

I think its a good callout, but as you have mentioned we may not have any benefit on the unit test.

At least it will run and cover the added code, that is still valuable.

I have some example code which I believe we can use to add as an integration test. You can find my changes at main...bhks:grafana-client:main#diff-dc47e20d0c4b3322afa473cbebd824e602265a21bc5b628bb4e2de5496d37a9a

I wanted to first see how my first PR goes and get an opportunity to learn what is the process and how we are thinking about this.

Adding a trimmed variant of your example programs to the examples folder will be well appreciated on behalf of another patch. Maybe, when not installing the whole shebang of what is shipped with aws-grafana, but using a trimmed-down version instead, for demonstration purposes, the total runtime (probably determined by download speed and -efficiency) can be reduced, so we can think about including it into corresponding integration tests.

For bringing in the test framework itself, I was planning to reuse existing code from Kotori and grafana-wtf.

Thank you for your commitment, I appreciate it very much.

@bhks
Copy link
Contributor Author

bhks commented Sep 13, 2023

At least it will run and cover the added code, that is still valuable.

Yeah let me find some time to add that test.

The total runtime (probably determined by download speed and -efficiency) can be reduced, so we can think about including it into corresponding integration tests.

Based on my testing the download is pretty fast specially the Grafana Labs marketplace binaries, which are available here https://grafana.com/api/plugins

with links like following:

      "packages": {
        "linux-amd64": {
          "md5": "4e2e3e22a568688b3141067804d76e6d",
          "sha256": "98d32c61fbb8cda667cfe64a097b57a762a77d5a0c90458d78885af9c97d5b30",
          "packageName": "linux-amd64",
          "downloadUrl": "/api/plugins/alexanderzobnin-zabbix-app/versions/4.4.1/download?os=linux&arch=amd64"
        }

So it might not cause issues, when we run the install tests in parallel to the other tests.

Co-authored-by: Andreas Motl <[email protected]>
@bhks
Copy link
Contributor Author

bhks commented Sep 13, 2023

Thanks for calling out on Unit tests, it still have values and helps set the right URLs and give us an opportunity to review our code carefully.

@bhks bhks force-pushed the upstream branch 2 times, most recently from 8b7cdc9 to 2cc6fd3 Compare September 13, 2023 23:51
@amotl
Copy link
Contributor

amotl commented Sep 15, 2023

Perfect, thank you so much!

@amotl amotl merged commit 0319de0 into grafana-toolbox:main Sep 15, 2023
6 checks passed
@amotl
Copy link
Contributor

amotl commented Sep 15, 2023

Dear Bhagwat. Your improvements have been included into release 3.8.0. Thank you again!

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

Successfully merging this pull request may close these issues.

2 participants