Code review comment for lp:~marcoceppi/charms/precise/wordpress/trunk

Revision history for this message
Mark Mims (mark-mims) wrote :

Ok, here's a static review: This is an _example_ charm we'll use in charmschools and demos, so I'll have more stuff to say than when reviewing a normal charm :)

# things to change

- love the config and especially the config-changed hook... it's very simple, organized, and demoable! Please put explicit values into config.yaml... please don't make me guess if I should put `Optimized` or `optimized`

- so `charm proof` gives `I: relation shared-fs has no hooks`. I understand the desire to add a more general relation than nfs, but please remove this metadata until it's implemented to keep it clean and simple for people to read (demo charm)

- instead of `files/wordpress/wp-content`, please pull themes and plugins from external repos as needed... it's just extra cruft in the charm and it seems like it could grow _considerably_ and change at a faster/different rate than the charm itself. We want this to be best-practice for charms (demo charm). Perhaps we should discuss this one in channel with the gang or on the list to get a consensus on this, but I'd certainly prefer configurable external repos.

- the `stop` hook references omgbackup... that's great but make it generic please

- I'd love to see some of your `files/charm` contents be actual templates where more stuff is externalized via config. e.g., ports and stuff. Well, ok... after looking some more, some of this stuff will *need* to be parametrized... i.e., there're urls littering the static config files. Those need to be in `config.yaml`

# discussion/recommendations

- it's unclear what `deets` is and why that's not coming from upstream repos

- `upgrade-charm` hook should maybe provide a way to change nginx config too?

Thanks man... totally excited about getting this pimped-out version in the main charm! we need more like this.

review: Needs Fixing

« Back to merge proposal