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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
