Code review comment for lp://staging/~chad.smith/charms/precise/storage/storage-nfs-python-provider

[8] There seem to be some mount and unmout functions in charmsupport,
should we use those? The answer could be no, I haven't looked closely
at them.

== nfs/data-relation-changed ==

[9] extra import sys

[10] you have a formatted variable "config_changed" that is used, but
a replacement is never run on it. Seems wrong

[11] General: what are all the calls to config-changed for?

[12] nfs: you could symlink stop and nfs-relation-departed, also get rid
of -broken?? I'm not sure why we need stop actually. I'd get rid of it
and start if possible.

== nova/config-changed ==

[13] remove jsonpipe install, and use python json module to read data in
nova_util.py. I get that this was a fast conversion, but that one
cleanup saves a package installation. :)

== nova/data-relation-* ==

[14] not sure we need a call to config_changed here, do we? I think I'm
seeing the problem. We need to install software in an install hook, but
if someone changes configuration that software will not be installed.
I'm thinking that we should symlink install and config-changed in all
these, and stop calling config_changed at the top of all relation hooks.

== nova/nova-util.py ==

[15] add docs

[16] get_volume_id(): your else here is dangerous, I think we should
fail if either the volume_id or name is not specified, or we create it.
Those are the only two options. Also, if the name doesn't return just
one volume, that should be an error.

[17] attach_nova_vlume(): combine first two sentinals to be check for
"available" status, and exit with error if not in that state?

[18] I would change most of the call() to check_call(), to get the
return code checked.

== nova/ ==

[19] same comments about start/stop hooks, and if they are needed. I
wonder if we just need to worry about the relation changed and departed?

--
David Britton <email address hidden>