Code review comment for ~woutervb/charm-grafana:bug/1873105

Revision history for this message
Alvaro Uria (aluria) wrote :

Hi, there are a few things to be reviewed:
* Makefile does not have a unittest target (probably because unit tests didn't exist). However, tox.ini does have a "unit" env. I think the Makefile misses the target, running "tox -e unit"

* Functional tests miss nagios-{{series}} and nrpe-{{series}} in the overlay.yaml.j2 template. It would be good to test when grafana gets related to nrpe, by checking that the check_grafana_http.cfg file exists in /etc/nagios/nrpe.d. There are examples of such check (file_stat) in ie. charm-openstack-service-checks

* Functional tests don't test a deployment of cs:grafana + juju upgrade-charm to the local version. I spotted a bug that can be seen at [1]. "nrpe-external-master.available" state gets removed and wipe_nrpe_checks is called. Looking at the debug lines, the http interface checks endpoint.website.joined state and removes website.available. I don't see any trace of calling the nrpe-external-master interface and do a similar thing (which OTOH only occurs when n-e-m-relation-{departed,broken} hooks are triggered - and debug lines don't show such run).

On this 3rd test (upgrade-charm) it would be could to verify that the old checks (check_grafana-server.cfg [apt] or check_snap.grafana.grafana.cfg [snap]) are removed (backward compatibility with a previous deployed environment). On fresh deploys, such checks won't exist, only check_grafana_http.cfg.

1. https://pastebin.ubuntu.com/p/2smQt4X3Wd/

review: Needs Fixing

« Back to merge proposal