First deploy charm, with tests.

First deploy charm, with tests.

First pass of the Juju GUI deploy charm, translated to Python from a
shell script prototype written by Kapil. Currently it only uses the
"improv" version of Juju, not the standard one.

It includes both unit and functional tests, the latter using the "test"
Jitsu command recently developed by Jim Baker. The README.txt file
shows instructions for running both kinds of tests.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/juju-gui/+merge/135213

Requires: https://code.launchpad.net/~benji/charms/precise/juju-gui/second/+merge/134370

(do not edit description out of merge proposal)

[frankban, 2012-11-20 18:47:42]

Please take a look.

[frankban, 2012-11-21 11:27:54]

Please take a look.

[benji, 2012-11-26 21:36:17]

This looks great.  I just had a few of small comments.

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

https://codereview.appspot.com/6842074/diff/3001/config/juju-api-improv.conf....
config/juju-api-improv.conf.template:2: author "Juju GUI Peeps"
Should this be "Canonical" or something similarly formal?

https://codereview.appspot.com/6842074/diff/3001/hooks/start
File hooks/start (right):

https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode23
hooks/start:23: CURRENT_DIR = os.getcwd()
Is it safe to assume that the CWD is what we expect it to be?  Would it be safer
to base our paths off the current file's location?

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#newcode23
tests/deploy.test:23: except urllib2.URLError as err:
Unfortunately urllib2.urlopen can throw several different kinds of exceptions in
different scenarios.

I suppose we could put a fairly generic "except: Exception" here instead or try
to enumerate them all.  I know that the set includes at least HTTPError (from
httplib) and IOError.  There are probably many more.

Honestly, do we really think that retrying for 30 seconds will make a material
difference?  If not, we could just leave the retries out altogether.  After all,
this isn't a long-running process that needs to stay up at all cost.

https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode57
tests/deploy.test:57: unittest.main(verbosity=2)
Doesn't the test runner inspect sys.argv so you can pass switches?  Imposing a
non-standard default is unwarranted.

[gary.poster, 2012-11-26 21:46:56]

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.templat...
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)

[gary.poster, 2012-11-26 21:57:00]

Another, different test failure:

JUJU_REPOSITORY=/home/gary/dev/repo ~/bin/jitsu test juju-gui --logdir /tmp
--no-bootstrap
2012-11-26 16:40:28,141 jitsu.test:INFO Running unit test: deploy.test
test_improv (__main__.DeployTest) ... 2012-11-26 16:55:28,145 jitsu.test:WARNING
Error running unit test deploy.test: 124 
2012-11-26 16:55:28,344 INFO 'status' command finished successfully
2012-11-26 16:55:28,360 jitsu.test:INFO Gathering logs for 0:localhost in
/tmp/deploy/0
ssh: connect to host localhost port 22: Connection refused
rsync: connection unexpectedly closed (0 bytes received so far) [Receiver]
rsync error: unexplained error (code 255) at io.c(605) [Receiver=3.0.9]
2012-11-26 16:55:28,364 jitsu.test:WARNING Error retrieving log /var/log/juju
from localhost: Command '['rsync', '--archive', '--compress', '--relative',
'--verbose', '-e', 'ssh', 'ubuntu@localhost:/var/log/juju', '/tmp/deploy/0']'
returned non-zero exit status 255
2012-11-26 16:55:28,365 jitsu.test:INFO Tearing down unit test: deploy.test
2012-11-26 16:55:28,365 jitsu.test:INFO Completed unit test: deploy.test

[frankban, 2012-11-28 11:54:59]

Thanks for the reviews Gary and Benji.

Tests on ec2 work without intermittent failures.
Tests on LXC run very slowly. We are landing this branch and I will keep on
launching test runs on ec2. I will add a card in case I experience further
fragilities.

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
On 2012/11/26 21:46:56, gary.poster wrote:
> 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.

Done.

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

https://codereview.appspot.com/6842074/diff/3001/config/juju-api-improv.conf....
config/juju-api-improv.conf.template:2: author "Juju GUI Peeps"
On 2012/11/26 21:36:17, benji wrote:
> Should this be "Canonical" or something similarly formal?

Done.

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.templat...
config/juju-gui.conf.template:10: exec nodejs %(juju_gui_dir)s/server.js
On 2012/11/26 21:46:56, gary.poster wrote:
> 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).

We agree the charm should start the production server. Created a card with your
comments in it: https://bugs.launchpad.net/juju-gui/+bug/1083545

https://codereview.appspot.com/6842074/diff/3001/hooks/start
File hooks/start (right):

https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode23
hooks/start:23: CURRENT_DIR = os.getcwd()
On 2012/11/26 21:36:17, benji wrote:
> Is it safe to assume that the CWD is what we expect it to be?  Would it be
safer
> to base our paths off the current file's location?

According to the Juju FAQs the hooks are always executed in the root directory
of the charm:
https://juju.ubuntu.com/docs/faq.html#what-directory-are-hooks-executed-in

https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode52
hooks/start:52: open_port(8081)
On 2012/11/26 21:46:56, gary.poster wrote:
> 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?

Done.

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#newcode23
tests/deploy.test:23: except urllib2.URLError as err:
On 2012/11/26 21:36:17, benji wrote:
> Unfortunately urllib2.urlopen can throw several different kinds of exceptions
in
> different scenarios.
> 
> I suppose we could put a fairly generic "except: Exception" here instead or
try
> to enumerate them all.  I know that the set includes at least HTTPError (from
> httplib) and IOError.  There are probably many more.
> 
> Honestly, do we really think that retrying for 30 seconds will make a material
> difference?  If not, we could just leave the retries out altogether.  After
all,
> this isn't a long-running process that needs to stay up at all cost.

Adding a timeout is needed because of having to wait for the service to be
exposed. Unfortunately it seems that we cannot check that using jitsu watch.
Catching URLError (and not other exceptions) allows us to do that. We do not
want to know if the response status is 200, we only want to know when the port
is reachable.
Added a comment to the code explaining this.

https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode38
tests/deploy.test:38: '--state', 'started', '--open-port', self.port)
On 2012/11/26 21:46:56, gary.poster wrote:
> 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)

This one seems a test runner failure. We have also experienced timeout errors
when running the tests using LXC. We added a note in README with instructions
about how to increase the jitsu test timeout.

https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode57
tests/deploy.test:57: unittest.main(verbosity=2)
On 2012/11/26 21:36:17, benji wrote:
> Doesn't the test runner inspect sys.argv so you can pass switches?  Imposing a
> non-standard default is unwarranted.

It seems not possible to pass arguments through the jitsu test command to the
test runner. A verbosity of 2 seems to us a good default to have enough feedback
about what is going on.

[frankban, 2012-11-28 12:02:26]

Please take a look.

[frankban, 2012-11-28 14:37:33]

*** Submitted:

First deploy charm, with tests.

First pass of the Juju GUI deploy charm, translated to Python from a
shell script prototype written by Kapil. Currently it only uses the
"improv" version of Juju, not the standard one.

It includes both unit and functional tests, the latter using the "test"
Jitsu command recently developed by Jim Baker. The README.txt file
shows instructions for running both kinds of tests.

R=benji, gary.poster
CC=
https://codereview.appspot.com/6842074