yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

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.template
File config/juju-api-improv.conf.template (right):

https://codereview.appspot.com/6842074/diff/3001/config/juju-api-improv.conf.template#newcode2
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.template#newcode10
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.

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.