Prometheus libvirt exporter charm

Code review comment for ~dmzoneill/charm-prometheus-libvirt-exporter:dev/q2-20

  1. Git
  2. lp:~dmzoneill/charm-prometheus-libvirt-exporter
  3. dev/q2-20
  4. Merge into master
Revision history for this message
Alvaro Uria (aluria) wrote :

I've added more comments inline. Other comments:
* The COMMIT_MSG has a typo (CHARM_BUILD_DIR)
* After cloning the repo, "make lint" failed because "mkdir report" is needed (I think it should be included by adding ./report/.keep or the extra flags in the flake8 command removed). Unit tests would not fail because of this, because pytest creates the folder if it does not exist.
* Unit tests should be added, and functional tests moved to a new folder ./tests/functional/
* If this MP won't add unit tests, I'd suggest removing all references to unit tests.

In general, the change looks good. I need to review a full deployment because the Zaza test does not include nova-compute to verify that metrics are retrieved from a running libvirt service.

review: Needs Fixing

« Back to merge proposal