Code review comment for lp:~lazypower/charms/trusty/docker/trunk

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Hey,

Got a chance to look this charm over, things look pretty good. Here are some general notes, monstly minor.

README: be sure to update the CS URI to point to the proper location. You reference a full manual at the github loc but don't include the link there, ah, I see the github version actually has this. Not sure why this is behind.

config.yaml: you might want to highlight the precedence between latest and version keys a little more.

unit_tests: If you don't have them I'd drop the directory. This is ok to me as you do have the integration tests. It does feel like the docker opts mgr should have unit tests to get that merged in though.

integration-tests: Do assertions around the relation live in a bundle outside this? You test basic runtime but not that the relations work.

hooks/install -> setup.py -> hooks.py -> playbook dance: Maybe more complicated than it needs to be, if you don't call into hooks after the basic apt-get work you can let config-changed handle the rest (as it deals with version/latest), might be a little cleaner, what do you think?

playbooks/*: Try as I might I don't find these any easier to read, write, modify than the other best practices we have, but I am all for trying to standardize on something. After some time with Ansible charms I would like to have a talk with the team and see how everyone feels it plays out in practice.

« Back to merge proposal