Return "unknown" if show version info is disabled in Grafana

[schmiddim]

Description

We disabled displaying the version info because of ridiculous security Resasons.

Checklist

• [ x ] 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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ Current head b9fe114 differs from pull request most recent head 7ee5d81. Consider uploading reports for the commit 7ee5d81 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
- 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.

[amotl]

Would returning None also be an option? It would be a more agnostic token for downstream users to act upon, for example when parsing the version string using verlib2, or friends.

[schmiddim]

Good Point! @amotl I updated the request.

[chintal]

Note that there is atleast one API section which relies on Grafana version information to figure out whether the interfaces are supported.

Example : https://github.com/panodata/grafana-client/blob/4bd145a78832a225ef83e304342f954ff6fba86c/grafana_client/elements/datasource.py#L177

Be aware that simply returning None might result in unexpected exceptions in these places until they are tweaked to handle this possibility.

[schmiddim]

Yes I know. I also have to work with lib elements where it's required. What else do you recommend? A Version not available Exception?

[chintal]

@schmiddim

Most of the version checks I've noticed seem to be related to deprecation warnings, and not for progressive upgradation of the interface. The version checks can be made insensitive to None versions where you find them (they already are in many places).

If grafana version isn't available, I think it's fair that the user creating that situation will need to take responsibility to deal with deprecation. So it should not be a problem to make that change, in my opinion.

The problem is going to be in going through and finding all of them. If @amotl is fine with this solution in principle, then I would recommend fixing it as and when you hit such a problem.

[schmiddim]

@chintal would be awesome. I also have to work with Library Elements and will fix things when I hit them.

[amotl]

Hi. Thanks for your conversation on this topic. I also wanted to ask if we do any feature dispatching by version number. Thanks for bringing that to the table, @chintal.

Would it be sensible to let the user set the Grafana version manually / make that obligatory, when it can't be inquired by the library itself, in order to satisfy all downstream use cases elegantly, and in order not to add additional complexity around this topic?

In this case, the version property will just need a corresponding setter, and maybe additional sanity checks, so the user will receive a proper exception when running the library without Grafana server version information.

wdyt?

[chintal]

@amotl

Allowing the user to set the version manually is probably fine. I do have these thoughts about it, though :

1. Setting the version manually should not be mandatory - even if the server does not report its version. If it isn't set, we simply don't check for deprecation. Documentation should also reflect this, and users should be discouraged from setting the version manually unless they know they need it.
2. Unless a clear use case is understood, a user should not be allowed / able to override the version reported by grafana (if it is available).
3. As a corollary to 2, if user code sets a grafana version, and grafana also reports its version, and grafana-client uses the grafana reported version silently, then all of a sudden the user code is running against a grafana / grafana-client version different from what it explicitly expects. This will lead to painful debugging triggered asynchronously when either grafana server configuration changes, or server version changes.
4. As an addendum to 3, if we don't allow the user to set version if grafana already reports it and raise an exception, then the same can occur. User code will start failing when seemingly distant infrastructure changes are made, even if the user code doesn't actually care about any of it and would otherwise have worked normally.

On the whole, my opinion is that it's a layer of complexity that creates as many problems as it solves. Note that this all applies because grafana-client passes on most API changes down to the user to deal with anyway. For example, with a number of API endpoints which are deprecated, we simply raise a deprecation warning / error. We don't instead make the 'new' API calls required to get the same information and return that instead. I think this is the 'safest' approach presently, but I don't know how well this approach will scale over extended periods of time and grafana versions. Things will get messy when there are small schema changes to the same endpoint's requests or responses.

[amotl]

Hi. Apologies that I lost a bit track of staying involved with this, and I am still a bit swamped. Do you all agree this patch should be merged as-is, and included into the next release?

If so, I will probably try to converge it soon. To do that, I would like to get a proper acknowledgment from the community (you!). Thank you very much in advance.

On the other hand, if you think the patch will need further adjustments, please let me know. Thank you again.

[chintal]

I think it can be merged. The change would not interfere with any currently working code.

In addition, changes should be made in other files where version is required. It can be probably be done separately as well, since that code probably isn't working under the no-version-available condition now anyway.

For example, at

grafana-client/grafana_client/elements/datasource.py

Line 177 in 4bd145a

if Version(self.api.version) > VERSION_10_2_2:

if Version(self.api.version) > VERSION_10_2_2:
            raise NotImplementedError("Deprecated since Grafana 10.2.3") 

can be changed to something like:

if self.api.version and Version(self.api.version) > VERSION_10_2_2:
            raise NotImplementedError("Deprecated since Grafana 10.2.3") 

[Garry1704]

Hey will this MR be merged in the next time? I ran into the same Problem with version Exception

[schmiddim]

Hi, I'm out of time at the moment for maintaing this PR. I will modify further locations and update the PR
Had been some time since my PR - sorry I don't know how I can fullfil the further requirements from the maintainers.

[amotl]

Hi again. Sorry so much for the delay. I will merge as-is, and run a release afterwards.

[amotl]

grafana-client 4.1.2, just published to PyPI, includes your improvement, @schmiddim. Thanks again!