yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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.