Create a landscape-server charm

[David Britton]

Create a landscape-server charm, which will deploy the landscape-server software.

This charm will need to be owned by 'landscape-charmers', just as the 'landscape-client' charm is currently set up.

[Cory Johns (johnsca)]

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.

[David Britton (dpb)]

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 authentic...

[David Britton (dpb)]

OK, just found the source of the flaky tests. There were a couple commits in our development branch that did not make it into the charm store review.

As we discussed on IRC, we had already seen and fixed these errors in the past, but the reason you didn't see them is we had not merged them correctly. Apologies!

I have merged just those changes up for inclusion. They are well tested, and it was simply a merge error that caused these commits to be missing. You should have 2 extra revisions now from when you reviewed originally. It's now up to r154.

Thanks!

[Charles Butler (lazypower)]

David,

Thank you for the rapid turn around with the review fixes. I've taken a look at the results and everything looks great. I'm going to promulgate the charm as is, with a request to have the deployments yaml moved into a bundle using the instructions: https://juju.ubuntu.com/docs/charms-bundles.html the process is straight forward and involves another branch with the bundles.yaml and a readme.

Since that's not a serious blocker for this charm, and the tests passed on all the substrates I've run it against (again, thank you for such a high standard of code coverage, this is a great example for future developers looking to reference a "how do I test my charm?")

The promulgation process will take about 15 to 30 minutes for the charm to be ingested into the store. I'll follow up on where the bug management location will be for the landscape team, and final resource link for the landscape charm in launchpad.

The finish line is in your sights! Congrats on making it into the store!

[Charles Butler (lazypower)]

Thanks again for an excellent addition to the juju charm store.

I've promulgated the landscape-server charm and it should be displaying in the charm store within the next 15 - 30 minutes.

Bugs may now be filed against the lp:charms/landscape-server charm
 https://bugs.launchpad.net/charms/precise/+source/landscape-server

View the charm code
https://code.launchpad.net/~landscape-charmers/charms/precise/landscape-server/trunk

~landscape-charmers are the branch owners and reviewers.

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>

Congratulations and thank you for your dedication and patience during the review process.