Comments inline.


>
> - I don't understand what expose_app is for, how does this differ from
> juju expose node-app?
>

It's actually not different, and seeing your other comment below (" juju
unexpose (and expose) aren't executed from the charm but instead by the
user") I've removed this.


> - Excellent use of app_user, however it's not being used in the upstart
> script. Consider using the following as a reference:
> https://gist.github.com/marcoceppi/8458344


Modified the upstart script according to this reference.

- I really like the way lib/charm.js is shaping up, I'm confident this will
> be a key part in helping other node.js authors write hooks/charms in
> node.js Along the same vein I think methods like installPackage,
> installApp, updateApp, configureApp, etc are fantastic, but don't seem
> inheriently a part of Charm itself. As an opinion it might be better served
> breaking the two apart. Charm to emulate all the core juju functions in
> node then a seperat App module with these methods.
>

This is actually a good point. I've moved out of lib/charm.js all methods
that are app related (now in lib/app.js)


>
> - The charm needs more logging to juju-log, most of the comments you have
> in the final callbacks are great, sending them to juju-log will notify the
> user what's going on and help debug issues
>

Agreed, added logging to juju-log.


>
> - juju unexpose (and expose) aren't executed from the charm but instead by
> the user, as a result the mongodb-relations-departed and stop hooks need to
> be updated. Instead the charm needs to tell juju /what/ports to open by
> calling `open-port` and `close-port` These won't explicitly open or close
> the ports, but tells juju what ports to open in the firewall when the user
> run juju expose/unexpose.
>

Done.


>
> # Knitpick
>
> - I really like the idea of a polling_schedule, as an alternative it might
> be nice to have a push_url, where the user could specify a token as a
> configuration option, then have a url like http://ip:2222/update?key=<key-they-set>
> so they could have github push notifications on updates and other
> repositories instead of a constant polling. Just an idea.
>
> - Having config/config.js where it is, I thought it was something the user
> filled out. I fear other users may also make this assumption. I'm not sure
> of a better place just making you aware of the perception.
>
> - You'll want to update the maintainer field in the metadata.yaml file
>

Done :)


>
> - Again, not sure if this is a common practice in node, but relation_set
> seems to take a list of keys and values, would it be better served as an
> object of key: val pairs?
>

Agreed, that's a better way.


>
> Sorry this review was so...breif...in words. I honestly love the direction
> of the charm so far and understand it's still under development. These were
> things that popped out at me doing a comb through the charm. Let me know if
> you have any question, happy to discuss these points further.
>
> I'm going to move the merge request to Work In Progress, feel free to
> email me when you'd like another look or just move the merge to "Needs
> Review"
> --
>
> https://code.launchpad.net/~dstroppa/charms/precise/node-app/refactor-hooks/+merge/200330
> You are the owner of lp:~dstroppa/charms/precise/node-app/refactor-hooks.
>