Juju Charms Collection

Bug #1310704
Comment #2

Comment 2 for bug 1310704

Revision history for this message

David Britton (dpb) wrote on 2014-04-24:

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 <email address hidden>

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 <david.britton@canonical.com>