Thanks for the review, I've added some comments and will push some changes shortly

On 01.07.20 17:52, Zachary Zehring wrote:
> Review: Needs Fixing
> 
> Added comments inline. 
> 
> Diff comments:
> 
>> diff --git a/reactive/grafana.py b/reactive/grafana.py
>> index c64743a..1b78d77 100644
>> --- a/reactive/grafana.py
>> +++ b/reactive/grafana.py
>> @@ -1006,22 +1006,59 @@ def hpwgen(passwd, salt):
>>      return hpasswd
>>  
>>  
>> -@when("grafana.started", "endpoint.dashboards.requests")
>> +@when("grafana.started", "endpoint.dashboards.has_requests")
>>  @when_not("grafana.change-block")
>>  def import_dashboards(dashboards):
>> -    for request in dashboards.new_requests:
>> +    kv = unitdata.kv()
>> +    dashboard_digests = kv.get("dash_digests", {})
>> +
>> +    def dash_import(name, dash):
> 
> Is there any reason that this is a nested function? I see it's really only to avoid passing in dashboard_digests. I'd prefer to move this out into it's own private function, as it's clunky to read here.

Reasoning behind making it nested is to hide it from outside use, as it's not meant to be used directly. 

>>          try:
>> -            dash = request.dashboard
>> -            if isinstance(request.dashboard, str):
>> -                dash = json.loads(request.dashboard)
>> -            success, reason = import_dashboard(dash, request.name)
>> +            if isinstance(dash, str):
>> +                # Safety: per the dashboard interface the dash should already
> 
> I personally really dislike comments that describe intention that could be communicated through code. This check and possible assignment could be abstracted into a private function like 'ensure_dash_is_dict' which would be readable, less code in top function, and you could remove this block comment. Comments are often not updated when code changes, leading to misleading documentation. You could put any pertinent documentation into the docstring of the resulting function, which should be true describing the intention of the function.

I'm ok with abstracting this into a sep. function, but would hate to lose the comment as this is meant to explain the reason why this is being done. 

>> +                # be de-serialized json, but older dashboard providers used
>> +                # to send a json string, so attempt to deserialize if we find
>> +                # a string
>> +                dash = json.loads(dash)
>> +            success, reason = import_dashboard(dash, name)
>> +            dashboard_digests[name] = digest
>>          except json.decoder.JSONDecodeError as error:
>>              success, reason = False, str(error)
>> -
>>          hookenv.log(
>> -            "import_dashboards via grafana-dashboards: {} [{}]".format(
>> -                success, reason
>> -            ),
>> -            hookenv.DEBUG,
>> +            "import_dashboards {}, digest {}: {} [{}]".format(
>> +                name, digest, success, reason
> 
> Since success is a boolean, this log message would be a bit awkward (like what exactly does true/false mean here). I'd adjust somewhat to be more clear in what just happened.

Fair

>> +            )
>>          )
>> +        return success, reason
>> +
>> +    # Check for updates
>> +    for existing_req in dashboards.all_requests:
>> +        dash = existing_req.dashboard
>> +        if isinstance(dash, str):
>> +            # Similar to the above: attempt to deserialize for older providers
> 
> Definitely need to abstract this since you're doing it twice.

Ok

>> +            dash = json.loads(dash)
>> +        digest = dash.get("digest")
>> +        if not digest:
> 
> Would it be possible to add all the possible paths for a request here, in one for loop? You could add an if check for digest in the request and an elif check for it's existence in dashboards.new_requests, then process accordingly. That way you can just loop through once and remove the hacky 'continue' logic.

I've handled updates and new requests separately as I wanted to make it very clear that new requests are going to be imported no matter what, independent of the existence of a digest or not, so as not to break the existing behaviour. I don't think 'continue' is particularly hacky, but I know that opinions differ here. 

>> +            # Backward compat: if we don't see a digest in the payload, it's
>> +            # coming from an older provider, skip import of an existing
>> +            # request
>> +            continue
>> +        name = existing_req.name
>> +        hookenv.log(
>> +            "import_dashboards: found existing dash for {}, "
>> +            "with digest {}".format(name, digest)
>> +        )
>> +        if dashboard_digests.get(name) != digest:
>> +            hookenv.log(
>> +                "import_dashboards: updated digest, old: {}, new: {}. "
>> +                "Importing dashboard {}".format(
>> +                    dashboard_digests.get(name), digest, name
>> +                )
>> +            )
>> +            dash_import(name, dash)
>> +    # Import new requests
>> +    for request in dashboards.new_requests:
>> +        success, reason = dash_import(request.name, request.dashboard)
>>          request.respond(success, reason)
>> +    # Save back digests
> 
> Unnecessary comment. Code is already describing what it's doing.
> 
>> +    kv.set("dash_digests", dashboard_digests)
>> diff --git a/tests/functional/tests/test_grafana.py b/tests/functional/tests/test_grafana.py
>> index 5f452d9..7e34ac8 100644
>> --- a/tests/functional/tests/test_grafana.py
>> +++ b/tests/functional/tests/test_grafana.py
>> @@ -34,6 +34,42 @@ class BaseGrafanaTest(unittest.TestCase):
>>          u = model.get_unit_from_name(unit)
>>          return u.workload_status_message
>>  
>> +    def get_admin_password(self):
> 
> Would some version of caching the password as an instance variable work here? Not sure if this gets instantiated upon every test (i.e. new unit so same password).

Good point, the admin password shouldn't change

>> +        action = model.run_action(
>> +            self.lead_unit_name, "get-admin-password", raise_on_failure=True,
>> +        )
>> +        passwd = action.data["results"]["password"]
>> +        return passwd
>> +
>> +    def get_and_check_dashboards(self, cond):
>> +        """Retrieve dashboard from grafana
>> +
>> +        Check if cond(dashboards) is true, otherwise retry up to TEST_TIMEOUT
>> +        """
>> +        passwd = self.get_admin_password()
>> +        timeout = time.time() + TEST_TIMEOUT
>> +        dashboards = []
>> +        while True:
>> +            req = requests.get(
>> +                "http://{host}:{port}"
>> +                "/api/search?dashboardIds".format(
>> +                    host=self.grafana_ip, port=DEFAULT_API_PORT
>> +                ),
>> +                auth=("admin", passwd),
>> +            )
>> +            self.assertTrue(req.status_code == 200)
>> +            dashboards = json.loads(req.text)
>> +            if time.time() > timeout or cond(dashboards):
>> +                # Break out if the time is up or our condition becomes true
> 
> Comment doesn't seem necessary. It's pretty clear from if condition what it's doing.
> 
>> +                break
>> +            # Wait before retrying
> 
> Also pretty unnecessary comment.
> 
>> +            time.sleep(30)
>> +        return dashboards
>> +
>> +    def remote_sed(self, unit, file, expr):
>> +        cmd = ["sed", "-i", "-e", expr, file]
>> +        model.run_on_unit(unit, " ".join(cmd))
>> +
>>  
>>  class CharmOperationTest(BaseGrafanaTest):
>>      """Verify operations."""
>> @@ -99,25 +135,11 @@ class CharmOperationTest(BaseGrafanaTest):
>>  
>>      def test_10_grafana_imported_dashboards(self):
>>          """Check that Grafana dashboards expected are there"""
>> -        action = model.run_action(
>> -            self.lead_unit_name, "get-admin-password", raise_on_failure=True,
>> -        )
>> -        passwd = action.data["results"]["password"]
>>          # Loop this, it takes a while for the data to arrive and dashboards
> 
> Incorrect comment based on new code as the loop lives in another function entirely. I'd just remove.
>>          # to populate
>> -        timeout = time.time() + TEST_TIMEOUT
>> -        dashboards = []
>> -        while time.time() < timeout and len(dashboards) < 4:
>> -            req = requests.get(
>> -                "http://{host}:{port}"
>> -                "/api/search?dashboardIds".format(
>> -                    host=self.grafana_ip, port=DEFAULT_API_PORT
>> -                ),
>> -                auth=("admin", passwd),
>> -            )
>> -            self.assertTrue(req.status_code == 200)
>> -            dashboards = json.loads(req.text)
>> -            time.sleep(30)
>> +        dashboards = self.get_and_check_dashboards(
>> +            lambda dashes: len(dashes) >= 4
>> +        )
>>          self.assertTrue(len(dashboards) >= 4)
>>          self.assertTrue(
>>              all(
>> @@ -127,6 +149,40 @@ class CharmOperationTest(BaseGrafanaTest):
>>              )
>>          )
>>  
>> +    def test_11_dashboard_upgrade(self):
>> +        """Test upgraded dashboards are imported."""
>> +        exporter = "prometheus-libvirt-exporter/0"
>> +        self.remote_sed(
>> +            exporter,
>> +            "/var/lib/juju/agents/unit-prometheus-libvirt-exporter-0/charm/"
>> +            "files/grafana-dashboards/libvirt.json",
>> +            "s/Libvirt/FunctionalTestMarker/",
>> +        )
>> +        model.run_on_unit(
>> +            exporter, "JUJU_HOOK_NAME=upgrade-charm ./hooks/upgrade-charm"
>> +        )
>> +        model.block_until_all_units_idle()
>> +
>> +        def chk(dashes):
>> +            marker_dashes = [
>> +                d for d in dashes if "FunctionalTestMarker" in d["title"]
>> +            ]
>> +            return len(dashes) >= 4 and len(marker_dashes) == 1
>> +
>> +        dashes = self.get_and_check_dashboards(chk)
>> +        self.assertTrue(chk(dashes))
>> +
>> +        self.remote_sed(
>> +            exporter,
>> +            "/var/lib/juju/agents/unit-prometheus-libvirt-exporter-0/charm/"
>> +            "files/grafana-dashboards/libvirt.json",
>> +            "s/FunctionalTestMarker/Libvirt/",
>> +        )
>> +        model.run_on_unit(
>> +            exporter, "JUJU_HOOK_NAME=upgrade-charm ./hooks/upgrade-charm"
>> +        )
>> +        model.block_until_all_units_idle()
> 
> Doesn't look like there's a check for the new updated dashboard back to Libvirt. Is this intended? Or is this to put it back to consistent state?


Indeed the intention is to put it back into it's original state in case more tests are being written that might assume this state

>> +
>>      def test_12_grafana_create_user(self):
>>          """Run the create-user action."""
>>          params = {
> 
>