Submitting 'clearwater-bono' charm for review

[Rob Day]

I'm following the process at https://jujucharms.com/docs/1.20/authors-charm-store#recommended-charms to submit my charm for review.

The charm's code is hosted on Github at https://github.com/Metaswitch/clearwater-juju/tree/08be1f07836c4623467891bc151fcf46da674ffd/charms/precise/clearwater-bono.

This is part of a wider group of charms (one charm for each of the six components of a Project Clearwater system), orchestrated together by the bundle at https://github.com/Metaswitch/clearwater-juju/blob/08be1f07836c4623467891bc151fcf46da674ffd/charms/bundles/clearwater/bundle/bundles.yaml.

I'm just submitting the clearwater-bono charm for review at this point - the other five charms are quite similar, so I think any feedback on this charm would apply to the others, so I think it makes more sense to just have this one reviewed than have six reviewers spend time giving us very similar feedback.

[Charles Butler (lazypower)]

Greetings Rob,

Thank you for the submission of the Clearwater Bono charm for review! I'm excited to take a look at these charms as I have not looked at them since TADHACK in 2014, and they were very early in their lifecycle at that point.

I've pulled the bono charm and given it a look, while doing so I came up with the following comments:

Overall the charm is in good shape. I really enjoyed seeing the yield of the efforts of your team. I particularly liked that you're re-using chef recipes to stand up the service. This really speaks to the reusability of effort here, and once the charm makes its way to the charm store - with your approval I'd like to blog about this.

The charm passes proof, which is great! I see all the bases have been covered with the copyright, readme, and an attractive icon.

There is one nit about the metadata: categories has recently been renamed to tags, and we have a list of white listed suggested tags for charms here: https://jujucharms.com/docs/stable/authors-charm-metadata

In the chef-solo installer, I see that you're fetching an installer for chef-solo from opscode, but did not perform hash validation or fetch it over https. This leaves the script subject to MITM attacks and potentially opens the issue of obtaining a half-complete script without the sum validation.

As called out in the review request, the charm makes use of the AWS Metadata url in `lib/config_script`, `lib/dns_hook` - while I understand the usecase for this, there's also no way for the charm to fall back when being deployed to a non AWS environment. As a fair amount of the charms make use of AWS services, it stands to reason that the charms are only intended to be deployed to Amazon's datacenters. What I suggest to ease any migration headaches in the future, is to provide a fallback of just using unit-get if polling that metadata url doesn't work, or alternatively making it configurable. I'm thinking that users deploying in a MAAS substrate without openstack/aws - or users that run across this and test it on say GCE - will find this behavior broken for them.

In the `hooks/upgrade-charm`, I see you're implicitly calling `hooks/config-changed`. - Config-changed is implicitly called by juju after running the upgrade-charm hook, and this results in config-changed being executed twice after upgrading a charm. If this is desired behavior, feel free to disregard this comment :)

I see that are no hooks for `dns-agent-relation-departed` or `dns-agent-relation-broken` - if i were to add/remove the relationship the service would have trouble recovering, as its already been configured against the dns provider. I would like to see the charm handle this case, and place the underlying service in maintenance mode if possible, pending the dns-service rejoining the relationship.

I would love to see some amulet tests included in the charm. While not listed as required for Precise charms, this will greatly help the effort of ensuring the charm wont suffer from bitrot moving forward we can get it under CI to do a pulse check on the code. There is information about getting started writing integration tests for your charm here: https://jujucharms.com/docs/d...

[Rob Day (rkd-u)]

Thanks for the feedback, Charles - that all seems sensible. Some of it we've cleaned up already since submitting the charm for review (e.g. the AWS metadata URL) - I'll look at addressing the other feedback and resubmitting in the next week or so.

[Rob Day (rkd-u)]

Hi Charles,

Apologies for the delay here, but we've now applied your feedback and pushed it to https://github.com/Metaswitch/clearwater-juju/tree/master/charms/precise/clearwater-bono.

Taking your feedback comments specifically:

- We've renamed categories to tags
- Fetching an installer for chef-solo - since this installer is open-source, we've just made it part of the charm to resolve this issue
- Use of the AWS Metadata URL - we've removed this now that the corresponding Juju fix is in, and tested on openStack
- `hooks/upgrade-charm` calling `hooks/config-changed` - we've removed this
- Hooks for `dns-agent-relation-departed` or `dns-agent-relation-broken` - we've changed our DNS provider to https://github.com/chuckbutler/DNS-Charm, and that doesn't yet implement a departed relationship (https://github.com/chuckbutler/DNS-Charm/issues/8)
- Amulet tests - we've added one, and noting your positive comments about reuse, we've just made it a lightweight wrapper around our existing test infrastructure

[Rob Day (rkd-u)]

Any update here?