Hi Francesco. This looks very good to me. Thank you! The README change is the only one I'd like to require for you to land this. If you do that, please consider this an automatic approval.
Beyond that, making the ports configurable would be my next preference if you want to do that (maybe port 80 should be the default now, with 443 being what we use once we switch to certificates?). Please file a bug and make a card instead, if you don't make the change for this branch. After that, I think my discussion of serving should probably be deferred, unless you really have a good way to attack it quickly. If you do defer it, please create a bug and card. https://codereview.appspot.com/6842074/diff/3001/README.txt File README.txt (right): https://codereview.appspot.com/6842074/diff/3001/README.txt#newcode62 README.txt:62: JUJU_REPOSITORY=/path/to/local/repo ~/bin/jitsu test juju-gui --logdir /tmp The instructions don't specify what a repo is. It would be nice to clarify that, and integrate the repo instructions into the functional test setup above. https://codereview.appspot.com/6842074/diff/3001/config/juju-gui.conf.template File config/juju-gui.conf.template (right): https://codereview.appspot.com/6842074/diff/3001/config/juju-gui.conf.template#newcode10 config/juju-gui.conf.template:10: exec nodejs %(juju_gui_dir)s/server.js This starts the debug server. Eventually, I think it would be better to have the default installation use the static production files, found in the build directory. Of course, later, we will actually want to have releases of the static files alone. We would probably want three charm config options then: deploy debug from branch, deploy production from branch, or deploy production from "release" (a zip file of the build directory). The default would then be the newest release. For now, what you have done is probably the right thing to do. The other easy alternative is to replace line 10 with these 2 lines. chdir /home/ubuntu/gui/build exec python -m SimpleHTTPServer 8080 The downside to that approach is that people will not be able to pass URLs around. In the longer term, we would install nginx or apache and configure it to serve our build, with our index.html as the 404 page (or similar). https://codereview.appspot.com/6842074/diff/3001/hooks/start File hooks/start (right): https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode51 hooks/start:51: open_port(8888) This port probably needs to be configurable, to prevent conflicts. That can be done separately if you want to file a bug. It's interesting that we expose this without waiting for the expose command. I think I understand the impulse--practicality and expedience. However, I lean towards pushing this to a separate "expose" handler, to reduce surprise and maintain Juju patterns. Maybe get a feel from Ben or Kapil, as Juju experts? I'm OK with this decision, but I'd like you to explore it a bit more before you commit to it. https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode52 hooks/start:52: open_port(8081) As above. I was wondering why we needed to expose improv until the obvious hit me in the head like a brick. Maybe we should include comments to specify which port is for which service? https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test File tests/deploy.test (right): https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode38 tests/deploy.test:38: '--state', 'started', '--open-port', self.port) This test failed for me once (see below), and passed for me once. I'm on Quantal, running Juju tests on a Precise LXC. If you don't see intermittent behavior, I'm OK with proceeding, but we should do so with caution--if anyone else reports intermittent problems, or if I see this again, we should dig in. ====================================================================== ERROR: test_improv (__main__.DeployTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "./deploy.test", line 38, in setUp '--state', 'started', '--open-port', self.port) File "/usr/lib/python2.7/dist-packages/shelltoolbox/__init__.py", line 133, in callable_command return run(*all_args) File "/usr/lib/python2.7/dist-packages/shelltoolbox/__init__.py", line 452, in run raise exception CalledProcessError: Command '['jitsu', 'watch', '--failfast', 'juju-gui', '--state', 'started', '--open-port', '8888']' returned non-zero exit status 1 ---------------------------------------------------------------------- Ran 1 test in 303.993s FAILED (errors=1) https://codereview.appspot.com/6842074/ -- https://code.launchpad.net/~frankban/charms/precise/juju-gui/juju-gui/+merge/135213 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/charms/precise/juju-gui/juju-gui 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
