On Thu, Apr 24, 2014 at 06:20:49PM -0000, Cory Johns wrote: > 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. Thanks! We have worked pretty hard on it. > > #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. OK, I cleaned up all but 2 warnings. license-file and repository are right now required for this charm to deploy. I know that is not best practice, but it's the state of things right now (since this is deploying a commercial product). I discussed this with you on IRC and you said we are good to go here. > > #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. I put in right at the top of the "USAGE" that this charm was useless on it's own and needed a bundle. I left in the deployer instructions but will move those out as soon as I have somewhere (a bundle) to point someone. :) > > 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. I've removed this. Thanks for the feedback. > > 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). I cleared up that we expect just a URL in a number of places. I'll file a bug against the charm that we should do better validation here. Thanks! > > 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. > I updated the instructions. We are planning test suite enhancements to fix this up automatically, thanks for the suggestions, that sounds like a good way to do it. > #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.) Thanks, will dig into those flaky tests. We have a couple ideas. Could you mention to us under what environments they failed? Is there a CI where these are tested automatically? Would be good to see that kind of thing. > #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. > Thanks for the review, I really appreciate it! -- David Britton