yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Hi Francesco,

Approved with fixes.  (I think that is one of the new things we're
supposed to say.  :)

I have a few comments, mainly documentation.  The charm looks very good.
  Reading through it I remembered how nice the python-shelltoolbox is!

Thanks.


https://codereview.appspot.com/6846132/diff/1/README.txt
File README.txt (right):

https://codereview.appspot.com/6846132/diff/1/README.txt#newcode71
README.txt:71:
Where does this command run the tests?  ec2?  Or whatever the default
environment is?  (Looks like the latter.)

You should state that so there is no confusion.

https://codereview.appspot.com/6846132/diff/1/config.yaml
File config.yaml (right):

https://codereview.appspot.com/6846132/diff/1/config.yaml#newcode23
config.yaml:23: description: |
I'm confused as to what this option causes to happen.  Do you mean it
connects to the improv script running on uistage.jujucharms.com?  I
think so but the description  could be more explicit.

https://codereview.appspot.com/6846132/diff/1/config/juju-api-improv.conf.template
File config/juju-api-improv.conf.template (right):

https://codereview.appspot.com/6846132/diff/1/config/juju-api-improv.conf.template#newcode2
config/juju-api-improv.conf.template:2: author "Canonical"
We are using 'improv' as if it has some inherent meaning, which I'm not
sure is true.  It is OK that Kapil called the script that but for us to
propagate it in user-facing documentation seems wrong.

https://codereview.appspot.com/6846132/

-- 
https://code.launchpad.net/~frankban/charms/precise/juju-gui/bug-1074412-real-env/+merge/137140
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/charms/precise/juju-gui/bug-1074412-real-env into lp:~juju-gui/charms/precise/juju-gui/trunk.