Add API wrapper for "Plugins"

bhks commented 1 year ago

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
[x] The patch follows the style guidelines of this project
[x] The patch has appropriate comments, particularly in hard-to-understand areas
[x] The documentation was updated corresponding to the patch
[x] I have performed a self-review of this patch

amotl commented 1 year ago

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.

[^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 GH-50.

bhks commented 1 year ago

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

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 :https://github.com/panodata/grafana-client/compare/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[bot] commented 1 year ago

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%> (ø)`

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

bhks commented 1 year ago

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 commented 1 year ago

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

amotl commented 1 year ago

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 https://github.com/panodata/grafana-client/compare/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 commented 1 year ago

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.

bhks commented 1 year ago

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.

amotl commented 1 year ago

Perfect, thank you so much!

amotl commented 1 year ago

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

grafana-toolbox / grafana-client