Comment 1 for bug 1310704

Hi, David.

Thank you for this great charm. I especially appreciate the excellent test coverage. I have a few minor suggestions below, mostly centered around improving the README.

#Proof

Running `charm proof` raises a few warnings about missing default values in config.yaml. These are not critical, as this charm will be deployed via a bundle instead of directly, but it would be nice to have sensible defaults for services, license-file, and repository.

#README:

The Overview section should be more explicit that this charm should not be deployed stand-alone, but instead deployed via the bundle, with a pointer to the bundle (or where it will be). Since someone may stumble on this charm in the store by itself, it should be clear from a first glance at the README that they should instead look at the bundle.

The README mentions a default free 10-seat license (with an empty license file), but it's unclear what to do about the repo-file for that case. If this functionality is not ready, perhaps it should be removed from the README for now. Either way, I think instructions could be added pointing to how to get an LDS license which clarify that the repo-file value will be provided along with the license key.

Also regarding the repo-file value, it would be a lot more user friendly if the user could just copy the entire line given on the license page (including "deb" and series). Myself and Charles both got tripped up by that during review. Ideally, the file would accept either format (whole deb line or just the URL).

Finally, the instructions for running the tests mention installing python-twisted-core, but I also had to install python-pyscopg2, python-mocker, and python-psutil to get them to run, so those should maybe be mentioned. Alternatively, you could add a 00-setup "test case" that installs all the required packages, and reference it in the README for manual setup prior to running the tests by hand, if so desired.

#Tests

We had a few hiccups with the test runs, occasionally getting errors with the message:

  psycopg2.OperationalError: FATAL: password authentication failed for user "landscape_maintenance"

These seemed intermittent, however, and possibly related to timing or resources on the cloud instances. I was able to get a full clean test run against LXC, and I know Charles got a clean run against MAAS. These failures are definitely worth looking in to, but I don't consider them blockers. (The specific test cases that I had fail were: test_update_security_db_cron, test_sync_lds_releases_cron, test_maintenance_cron, and test_maas_poller_cron; full stack traces can be found at http://pastebin.ubuntu.com/7324058/ or upon request.)

#Summary

The bundle / deployer file is good, the tests are very good, and the instructions, while they could benefit from a bit of clarification, are good and easy to follow. I give this my +1 and look forward to seeing this charm and the associated bundle available on the store.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>

Thanks again.