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