Charm needed: Ghost blogging platform

[Jorge Castro]

http://www.howtoinstallghost.com/

[Jeff Pihach (hatch)]

Here is the initial commit of the Ghost charm:

https://code.launchpad.net/~hatch/charms/precise/ghost/trunk

It deploys the Ghost blog and has an interface to connect to MySQL

[Marco Ceppi (marcoceppi)]

Hi Jeff, thanks for waiting patiently while we review your charm! Sorry it's taken so long to get to, but the first cut is looking great - please see my review below:

# Proof

These are the results run from charm-tools, available in ppa:juju/stable, and executed using `juju charm proof` in the charm directory:

> I: all charms should provide at least one thing

The first error is more of an informational piece, though all charms /should/ provide something. In this case I think the ghost charm is a perfect candidate to provide a relation using the HTTP interface. Doing so will allow it to be attached to loadbalancers and other charms which consume the http interface.

> E: config.yaml: type of option config_file is specified as string, but the type of the default value is NoneType

This is indicating that having null as the default of a string type config is a mismatch. Use an empty string "" to indicate a null match.

W: config.yaml: option config_key does not have the keys: default, type

This indicates that the config_key configuration option is missing the default and type keys. These are expected for all configuration options.

# Review

Continuing with config.yaml, if items are truly not implemented don't include them in the config.yaml. Remove them, or comment them out, so they're just not listed at all instead of having them listed as (TODO) in the description.

Given the release string pattern, it'd be stellar to add a "release" or "version" configuration option so users could specify what version of ghost they wanted to use.

Install hook fails idempotency checks, because it's moving assets instead of copying.

You're upstart job runs ghost as root, when possible we recommend all services run as a normal user. http://upstart.ubuntu.com/cookbook/#setgid and http://upstart.ubuntu.com/cookbook/#setuid for how to achieve this in upstart.

Start hook runs npm start & and opens port. Shouldn't it call the upstart job instead? Also is ghost usable without a MySQL database? If not, the open port should happen after database-relation-changed is run.

Also, just noticed the configureSqlite, very cool. However, this will cause a problem and is not documented in the README. If you set up the blog, then add MySQL later, you'll lose your data. Same goes if you setup blog with MySQL and later remove the MySQL relation.

There's a database-relation-changed file that I can't figure out if it's ever called. If it's not used, just remove it. db-relation-changed seems to achieve the same thing in much fewer lines.

The README is super light on details, you'll want to document each of the configuration options more in depth, how the charm operates at different levels, how to add MySQL database and the ramifications of this, how to scale the service if applicable. README's should be written with the idea that "I'm a user, I just installed the GUI thing, and I want to deploy this, but I have no idea what I'm doing".

# Knitpicks

In line 7 of database-relation-changed, you exec touch, would it be better to use native V8 functions for this?

Here's a slightly more simplified, upstart-esque, init file: http://paste.ubuntu.com/6703577/

As you may well know...

[Marco Ceppi (marcoceppi)]

Thank you so much for your submission! I've moved this bug to Incomplete as a result of the above review. Once you've addressed the issues above either with code changes or comments, and wish to have another review, simply move the bug back to either "New" or "Fix Committed" to have it added back to the review queue.

There's not much left to do, the charm is really looking good. We appreciate your submission so far and look forward to the next round of reviews!

[Benjamin Saller (bcsaller)]

Thank you for this. I'd like to see this land in the story sooner than later but to my mind there is one central issue that is a blocker still.

It is laudable to try to configure the database based on the presence of the database relation, however the way this is done now it seems to represent a way to lose data (start with sqlite, blog for a while, add a mysql relation later). There is no mirgation. I'd suggest rather than do it this way make it a config setting and then make the service start depend on having a relation to the mysql database and all the valid keys before starting. If the config setting is mutated after the service is started you can throw a warning or an error indicating that the operation would result in data lose and isn't supported. start would be a noop and you'd check conditions in both config-changed and relation-changed to decide to start.

For config.url some ideas, if there is nothing on the balance interface the url can safely be defaulted to the unit-get public-address, it is only if something binds to that interface that you need to use/set url. In those cases one option, if the config.url isn't set, is to default it to the <public-ip>.xip.io of the remote balancer endpoint as that should resolve to a working deployment.

Knitpics

Is there a reason not to default to port 80? Even if you put a proxy in front of this that should be fine. Anyone experimenting with this is likely to want the default to be to serve traffic on the defautl port.

What is the use-case for not using host: 0.0.0.0 as the bind address, shouldn't that be fine? Can you drop the option?

Thanks for sticking with this, if any of these suggestions seem out of place I'm happy to talk them over, here or online.

[Jeff Pihach (hatch)]

Thanks for the review Ben.

As we discussed on IRC I'm going to leave the current SQLite and MySQL database interactions intact so that the experience is as close to using the default ghost blog installation set up as possible but I will promote the warning that the information will need to be manually transferred over in the readme. Once this lands I'll follow it up with bundles which have mysql and haproxy by default which I will then suggest as the recommended way to deploy this charm.

Also discussed in IRC is that I don't need internally resolvable addresses so the xip.io is not necessary. If someone has an issue with this we can revisit it at that time.

Knitpicks

It wasn't defaulting to port 80 to keep in line with the defaults of the Ghost install but I can change that.

I can't think of a reason where changing the host from 0.0.0.0 would be desired but I figured I might as well leave the option in there in the event someone does find it useful.

I'll update the charm and re-submit shortly.

[Jeff Pihach (hatch)]

After looking into defaulting to port 80, I would either need to include a server like nginx in the charm or make Ghost run as root which was an issue in a previous review so it's being left at the default ghost port. I've updated the charm to emphasize the SQLite to MySQL conversion issues and updated the version of Ghost.

[José Antonio Rey (jose)]

Hey Jeff,

I think setcap may be able to help you to listen in port 80 without having to be root. May be worth taking a look at.

[Charles Butler (lazypower)]

Ghost running on non port 80 is not a problem. It's fine considering Ghost is running as "middleware" and should ideally live behind a web daemon. Regardless if that web daemon lives on the host or on another unit (such as haproxy, or the apache2 charm)

</my 2 cents>

[Charles Butler (lazypower)]

Greetings Jeff,

Thank you again for your patience during the review process.

I've thoroughly reviewed the charm and it appears that everything I ran into as a potential blocker was already documented in the README. You've done an excellent job of ensuring that enough information is being conveyed to the user of the charm that this is just a middleware charm, you have manual steps to perform to complete the setup of installing an administrator.

# Knitpick

I'd like to suggest options to the charm user, if not a subordinate or a few configuration "roles" per-say of an MTA for ghost, as the bright yellow box after installation leaves me to feel that the installation is 90% complete, but not fully triaged by the charm or it's documentation.

Other than the above knitpick I don't see anything really blocking this charm from being accepted to the charm store. I'll be posting this and will follow up on this bug shortly with the bug tracker resource. Thanks again for all the effort that has gone into this charm.

All the best!

[Charles Butler (lazypower)]

Attached is the official bug tracker for the Ghost Charm as provided by the store:

https://bugs.launchpad.net/charms/+source/ghost

I've promulgated the charm and it should be available from the charm store at jujucharms.com within the next 20 minutes.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.