fix(grafana_datasource): orgId by name if defined to compare diff

[Nemental]

SUMMARY

If org_name is set instead of org_id, comparing the diff (after/before) to determine whether something has changed or not does not work.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

• module grafana_datasource

ADDITIONAL INFORMATION

    "diff": {
        "after": {
            "access": "proxy",
            "basicAuth": false,
            "database": "",
            "isDefault": false,
            "jsonData": {
                "tlsAuth": false,
                "tlsAuthWithCACert": false
            },
            "name": "Loki",
            "orgId": 1, # <-- datasource left unchanged in orgId 6 but diff uses default value of org_id (1) to compare so it's "changed"
            "type": "loki",
            "url": "http://loki-stack-loki-stack:3100",
            "user": "",
            "withCredentials": false
        },
        "before": {
            "access": "proxy",
            "basicAuth": false,
            "database": "",
            "isDefault": false,
            "jsonData": {
                "tlsAuth": false,
                "tlsAuthWithCACert": false
            },
            "name": "Loki",
            "orgId": 6,
            "type": "loki",
            "url": "http://loki-stack-loki-stack:3100",
            "user": "",
            "withCredentials": false
        }
    },

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (879e206) 69.25% compared to head (90f9783) 70.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   69.25%   70.82%   +1.56%     
==========================================
  Files           9       18       +9     
  Lines        1184     1878     +694     
  Branches      263      326      +63     
==========================================
+ Hits          820     1330     +510     
- Misses        212      404     +192     
+ Partials      152      144       -8     

Flag	Coverage Δ
integration	69.45% <100.00%> (+0.19%)	⬆️
sanity	23.53% <20.00%> (?)
units	65.18% <20.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[rndmh3ro]

I actually liked your first try (41596d3) better, because there you didn't redefine params["org_id"].

And now that I think of it: Now we check twice, if org_name is set. Could we define a new function for this in the GrafanaInterface-class, e.g. get_org_id and use it at these two places?

Aside, for another issue: We obviously need tests for idempotency.

[Nemental]

@rndmh3ro thank you for your review! In the GrafanaInterface class is already a function called organization_by_name, which was implemented in #332. You can check my recent changes. Maybe it's a solution we can go with.

[rndmh3ro]

This is what I had in mind:

diff --git a/plugins/modules/grafana_datasource.py b/plugins/modules/grafana_datasource.py
index 910e16b..3784999 100644
--- a/plugins/modules/grafana_datasource.py
+++ b/plugins/modules/grafana_datasource.py
@@ -688,6 +688,7 @@ class GrafanaInterface(object):
     def __init__(self, module):
         self._module = module
         self.grafana_url = base.clean_url(module.params.get("url"))
+        self.org_id = None
         # {{{ Authentication header
         self.headers = {"Content-Type": "application/json"}
         if module.params.get("grafana_api_key", None):
@@ -698,12 +699,12 @@ class GrafanaInterface(object):
             self.headers["Authorization"] = basic_auth_header(
                 module.params["url_username"], module.params["url_password"]
             )
-            org_id = (
+            self.org_id = (
                 self.organization_by_name(module.params["org_name"])
                 if module.params["org_name"]
                 else module.params["org_id"]
             )
-            self.switch_organization(org_id)
+            self.switch_organization(self.org_id)
         # }}}

     def _send_request(self, url, data=None, headers=None, method="GET"):
@@ -923,12 +924,7 @@ def main():
     ds = grafana_iface.datasource_by_name(name)

     if state == "present":
-        org_id = (
-            grafana_iface.organization_by_name(module.params["org_name"])
-            if module.params["org_name"]
-            else module.params["org_id"]
-        )
-        payload = get_datasource_payload(module.params, org_id)
+        payload = get_datasource_payload(module.params, grafana_iface.org_id)
         if ds is None:
             grafana_iface.create_datasource(payload)
             ds = grafana_iface.datasource_by_name(name)

This way org_id is only defined in the init-function of the class. with self.org_id we can then use it outside of the class in the payload function.

I think, this should work. My local tests are failing with another error, so can you please test this?

[Nemental]

@rndmh3ro Yes it works :) Great suggestion to define the variable for org_id this way. Thank you.