Code review comment for lp:~frankban/charms/precise/juju-gui/trusty-charm

Revision history for this message
Brad Crittenden (bac) wrote :

LGTM.
Will now QA.

https://codereview.appspot.com/88100044/diff/1/HACKING.md
File HACKING.md (left):

https://codereview.appspot.com/88100044/diff/1/HACKING.md#oldcode23
HACKING.md:23: do not yet have an environment defined, the Jitsu command
"setup-environment"
yay, death to jitsu

https://codereview.appspot.com/88100044/diff/1/HACKING.md
File HACKING.md (right):

https://codereview.appspot.com/88100044/diff/1/HACKING.md#newcode24
HACKING.md:24: way to get started. See
https://pypi.python.org/pypi/juju-quickstart.
Do you prefer pypi to the package just because we have tighter control
over it and it may be more up-to-date?

https://codereview.appspot.com/88100044/diff/1/HACKING.md#newcode62
HACKING.md:62: version the charm will be deployed by functional tests.
For instance, to test
This sentence ends clumsily. How about

Since functional tests deploy the charm in the
bootstrap node, setting the default series also selects which series
version the charm will be deployed by functional tests.

https://codereview.appspot.com/88100044/diff/1/HACKING.md#newcode66
HACKING.md:66: Long story short, to run both unit and functional the
tests:
s/the tests/tests/

https://codereview.appspot.com/88100044/diff/1/HACKING.md#newcode128
HACKING.md:128: The `make deploy` command also support specifying the OS
version used to deploy
s/support/supports/

https://codereview.appspot.com/88100044/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/88100044/diff/1/Makefile#newcode83
Makefile:83: @echo 'make deploy [JUJU_ENV="my-juju-env"]
[SERIES="trusty"] - Deploy and'
Shouldn't this say SERIES="precise|trusty" or do you just include trusty
because precise is the default?

https://codereview.appspot.com/88100044/diff/1/README.md
File README.md (right):

https://codereview.appspot.com/88100044/diff/1/README.md#newcode47
README.md:47: add additional machines (i.e. the GUI is installed in the
bootstrap node).
Two parenthetic i.e. clauses in one sentence is a red flag. How about:

When possible, Quickstart conserves resources by installing the GUI on
the bootstrap node. This colocation is not possible when using a local
(LXC) environment.

https://codereview.appspot.com/88100044/diff/1/README.md#newcode165
README.md:165: While the Juju GUI itself is a client side JavaScript
application, the charm
trivial: client-side

https://codereview.appspot.com/88100044/diff/1/README.md#newcode166
README.md:166: installation also involves configuring a starting a GUI
server, which is
configuring and starting?

https://codereview.appspot.com/88100044/diff/1/README.md#newcode205
README.md:205: * The legacy server is no longer supported/tested staring
from trusty.
typo:starting

https://codereview.appspot.com/88100044/

« Back to merge proposal