yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Hi Francesco.  This looks good to me.  Thank you.  I have a very few
actionable comments.

I did try to run the tests and they timed out for me again, even with
the gigantic timeout. :-(

$ JUJU_REPOSITORY=/home/gary/dev/repo ~/bin/jitsu test juju-gui --logdir
/tmp --timeout 40m
2012-11-30 08:24:20,953 jitsu.test:INFO Running unit test: deploy.test
2012-11-30 08:24:20,953 jitsu.test:INFO Bootstrapping default
environment
2012-11-30 08:24:21,118 WARNING ssl-hostname-verification is disabled
for this environment
2012-11-30 08:24:21,118 WARNING EC2 API calls encrypted but not
authenticated
2012-11-30 08:24:21,118 WARNING S3 API calls encrypted but not
authenticated
2012-11-30 08:24:21,118 WARNING Ubuntu Cloud Image lookups encrypted but
not authenticated
2012-11-30 08:24:21,120 INFO Bootstrapping environment 'ec2' (origin:
ppa type: ec2)...
2012-11-30 08:24:23,439 INFO 'bootstrap' command finished successfully
test_api_agent (__main__.DeployTest) ... 2012-11-30 09:04:23,464
jitsu.test:WARNING Error running unit test deploy.test: 124

I put a card in the board for someone to try and figure out what the
heck is going on.  This is annoying.  But anyway, for your branch, I'm
ok with landing it despite this, given your diligence of running the
tests 10 times in a row.

Thanks

Gary


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

https://codereview.appspot.com/6846132/diff/1/config.yaml#newcode16
config.yaml:16: default: 8081
Breaking news: Kapil has requested 8080 (as of yesterday).  We will
serve the GUI from port 80 after bug 1083545, and 443 (by default) after
bug 1083920.  Might as well go ahead and change to the desired 8080 here
now, while we are thinking of it.

https://codereview.appspot.com/6846132/diff/1/config.yaml#newcode22
config.yaml:22: staging-environment:
nice, good idea.

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

https://codereview.appspot.com/6846132/diff/1/config/juju-api-improv.conf.template#oldcode9
config/juju-api-improv.conf.template:9: exec /usr/bin/python
%(juju_dir)s/improv.py -f %(juju_dir)s/sample.json &>
/tmp/improv-agent.output
Why don't you stash the output any more?  Doesn't seem critical also
seems reasonably nice.

https://codereview.appspot.com/6846132/diff/1/hooks/install
File hooks/install (right):

https://codereview.appspot.com/6846132/diff/1/hooks/install#newcode27
hooks/install:27: def fetch(juju_gui_branch, juju_api_branch):
Good.

https://codereview.appspot.com/6846132/diff/1/hooks/start
File hooks/start (right):

https://codereview.appspot.com/6846132/diff/1/hooks/start#newcode39
hooks/start:39: log('Generating the Juju-GUI configuration file.')
Niggly niggle: you use "Juju GUI" in your new log message.  I like that
better than "Juju-GUI" myself, but might as well make things consistent.
  If you feel like it. :-)

https://codereview.appspot.com/6846132/diff/1/hooks/start#newcode49
hooks/start:49: def start_improv(juju_api_port, staging_env):
Nice job separating things out into functions.  Thank you.

https://codereview.appspot.com/6846132/diff/1/tests/deploy.test
File tests/deploy.test (right):

https://codereview.appspot.com/6846132/diff/1/tests/deploy.test#newcode88
tests/deploy.test:88: self.addCleanup(
Please add this before the test itself (check_service in this case).
This means that if the test fails, stop_services still is called.  It
also is nice that the actual test is the last line in the function IMO,
but that's less important.

https://codereview.appspot.com/6846132/diff/1/tests/deploy.test#newcode98
tests/deploy.test:98: self.addCleanup(
Please add this before the test itself

https://codereview.appspot.com/6846132/diff/1/tests/deploy.test#newcode109
tests/deploy.test:109: self.addCleanup(
Please add this before the test itself

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.