Comment 5 for bug 911833

Revision history for this message
Marco Ceppi (marcoceppi) wrote :

# Blockers
- Improper category. From my understanding of this services it's not misc it's an application
- I understand charm icons are pretty new, and we don't have the process fully documented yet (It's something that will be landing soon). However, the charm icon you've provided doesn't follow the charm icon guidelines. The guidelines for this can be found here: https://docs.google.com/document/d/1hBTH_7RLUYMV-VhZOGxb-bFHICXyVRU4t8L79Dlea80/pub There's a template you can use within the latest version of charm tools (so if you run `charm create foo` in a temporary directory you can copy the icon.svg to this charm and use it as your base). Let me know if you require additional help with the icon as it is a very new policy.

This is the biggest problem by far. I'm going to roll these two points in to one as they are closely related. Currently, if you just run juju deploy tt-rss then juju expose tt-rss the unit is exposed but not actually set up. So you get a broken exposed service. Also, if you ever change the configuration option for version of tt-rss nothing happens. The expectation with Juju is every time you change a configuration option the charm should be able to react to that as the user expected. So as a user if I change the "url" (ie. the source) of the service I expect the charm to actually reconfigure itself to use that. How practical that is with this service is something you'll need to make a note of. If the upstream is known to break backwards compatibility of not provide a way to upgrade schema between releases then you will need to clearly document in the README file and config description that this configuration option can only be set during deployment and will not change the service. This really isn't ideal but there are some instances where it must be that way. Back to my previous point, where the charm is "broken" and being exposed. Ideally you don't want to open-port until the service is ready to be exposed. This ultimately should happen after db-relation-changed has succeeded. So, if you choose not to address changing versions of the charm on the fly via the url/source configuration option, you'll at a minimum need to move the open-port command to the end of db-relation-changed. If you want to go the route of upgrading versions, which I'd recommend for a complete charm if possible, you'll want to move a lot of the install logic to the config-changed hook and persist mysql authentication information in separate file so you can constantly re-create the config.php file for the service.

# Nitpicks
- The description is pretty light. We typically like for the description of the charm to be a brief summary of what the service is and what happens in this charm for the service. In doing so it helps expose users to the charm when searching for similar items in the charm store. An example of a good description is the MySQL Charm: http://jujucharms.com/charms/precise/mysql
- The configuration option "url" is really better named as "source" as it's defining where to pull the source of this charm and not so much the URL which implies something that the service will be providing. I'd recommend changing this though it's truly a nitpick rather than a vital blocker
- "Best Practices" would dictate writing an upstart script instead of an init script. There's nothing really actionable on this, the init script is fine for this, but for future reference if you're going to write a start up script consider instead using upstart.

# Conclusion
I know this was quite a bit of feedback, but honestly you've got a great start to this charm! Of all the things, the last paragraph of blockers is the biggest thing which I tried my best to describe while staying brief. If you need me to explain these points forward or go in to more details for potential resolutions, please let me know! Otherwise great start, I look forward to the next revision.