Code review comment for lp:~frankban/charms/precise/juju-gui/improve-error-handling

Reviewers: mp+194398_code.launchpad.net,

Message:
Please take a look.

Description:
Improve bundle deployment error handling.

Workaround for http://bugs.python.org/issue1692335.

Include a customized jujuclient tarball in the
charm, The diff with the original is here:
http://pastebin.ubuntu.com/6377430/

Add a first fix to the testing environment creation
so that the local dependencies are installed
when available.

Also fixed the headers sent by the info handler.

Tests: make unittest

QA:
I used this bundle: http://pastebin.ubuntu.com/6377441/
to live test the branch.
Bootstrap a juju environment, run `make deploy` and then
switch the gui source to lp:juju-gui.
When everything is ready, try to deploy the bundle above:
it will fail because it includes local charms, but the
server will not explode, and it will be possible
to deploy other valid bundles after the first one.
The GUI user does not receive notifications, and that's
normal since deployments are not yet watched by the GUI.
Go to https://[GUI URL]/gui-server-info and you should
find the deployment status to be completed with the
expected error.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/improve-error-handling/+merge/194398

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/23000043/

Affected files (+44, -4 lines):
   A [revision details]
   M deps/jujuclient-0.13.tar.gz
   M hooks/utils.py
   M revision
   M server/guiserver/handlers.py
   M server/guiserver/tests/bundles/test_base.py
   M server/guiserver/tests/test_handlers.py
   M tests/requirements.pip

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision:
<email address hidden>

Index: revision
=== modified file 'revision'
--- revision 2013-11-07 11:18:20 +0000
+++ revision 2013-11-07 16:02:01 +0000
@@ -1,1 +1,1 @@
-93
+94

Index: deps/jujuclient-0.13.tar.gz
=== modified file 'deps/jujuclient-0.13.tar.gz'
Binary files deps/jujuclient-0.13.tar.gz 2013-11-06 18:48:54 +0000 and
deps/jujuclient-0.13.tar.gz 2013-11-07 17:27:20 +0000 differ

Index: hooks/utils.py
=== modified file 'hooks/utils.py'
--- hooks/utils.py 2013-11-07 14:41:27 +0000
+++ hooks/utils.py 2013-11-07 17:27:20 +0000
@@ -119,6 +119,9 @@
      'futures-2.1.4.tar.gz',
      'tornado-3.1.1.tar.gz',
      'websocket-client-0.12.0.tar.gz',
+ # XXX frankban 2013-11-07: we are currently using a customized
jujuclient
+ # version built from this branch:
+ # lp:~frankban/python-jujuclient/pickable-enverror.
      'jujuclient-0.13.tar.gz',
      # XXX frankban 2013-11-07: we are currently using a customized deployer
      # version built from this branch:
lp:~frankban/juju-deployer/guienv-fixes.

Index: server/guiserver/handlers.py
=== modified file 'server/guiserver/handlers.py'
--- server/guiserver/handlers.py 2013-10-17 12:47:15 +0000
+++ server/guiserver/handlers.py 2013-11-07 18:04:32 +0000
@@ -22,7 +22,6 @@
  import time

  from tornado import (
- escape,
      gen,
      web,
      websocket,
@@ -227,7 +226,10 @@
      def get(self):
          """Handle GET requests."""
          info = self.get_info(self.application.settings)
- self.write(escape.json_encode(info))
+ # In Tornado Web handlers, just writing a dict JSON encodes the
+ # response contents and sets the proper content type header
+ # (application/json; charset=UTF-8).
+ self.write(info)

  class HttpsRedirectHandler(web.RequestHandler):

Index: server/guiserver/tests/test_handlers.py
=== modified file 'server/guiserver/tests/test_handlers.py'
--- server/guiserver/tests/test_handlers.py 2013-10-17 12:47:15 +0000
+++ server/guiserver/tests/test_handlers.py 2013-11-07 18:01:12 +0000
@@ -431,6 +431,9 @@
          }
          response = self.fetch('/info')
          self.assertEqual(200, response.code)
+ self.assertEqual(
+ 'application/json; charset=UTF-8',
+ response.headers['Content-Type'])
          info = escape.json_decode(response.body)
          self.assertEqual(expected, info)

Index: server/guiserver/tests/bundles/test_base.py
=== modified file 'server/guiserver/tests/bundles/test_base.py'
--- server/guiserver/tests/bundles/test_base.py 2013-11-07 11:17:54 +0000
+++ server/guiserver/tests/bundles/test_base.py 2013-11-07 17:27:20 +0000
@@ -17,6 +17,7 @@
  """Tests for the bundle deployment base objects."""

  from deployer import cli as deployer_cli
+import jujuclient
  import mock
  from tornado import gen
  from tornado.testing import(
@@ -32,6 +33,15 @@
  from guiserver.tests import helpers

+def import_bundle_mock(apiurl, password, name, bundle, options):
+ """Used to test bundle deployment failures.
+
+ This function is defined at module level so that it can be easily
pickled
+ and reused in another process.
+ """
+ raise jujuclient.EnvError({'Error': 'bad wolf'})
+
+
  @mock.patch('time.time', mock.Mock(return_value=42))
  class TestDeployer(helpers.BundlesTestMixin, AsyncTestCase):

@@ -205,6 +215,26 @@
          # Wait for the deployment to be completed.
          self.wait()

+ def test_import_bundle_exception_propagation(self):
+ # An EnvError is correctly propagated from the separate process to
the
+ # main thread.
+ deployer = self.make_deployer()
+ import_bundle_path
= 'guiserver.bundles.base.blocking.import_bundle'
+ with mock.patch(import_bundle_path, import_bundle_mock):
+ deployer.import_bundle(
+ self.user, 'bundle', self.bundle, test_callback=self.stop)
+ # Wait for the deployment to be completed.
+ self.wait()
+ status = deployer.status()
+ self.assertEqual(1, len(status))
+ expected = {
+ 'DeploymentId': 0,
+ 'Status': utils.COMPLETED,
+ 'Error': "<Env Error - Details:\n { 'Error': 'bad wolf'}\n
>",
+ 'Time': 42,
+ }
+ self.assertEqual(expected, status[0])
+
      def test_invalid_watcher(self):
          # None is returned if the watcher id is not valid.
          deployer = self.make_deployer()

Index: tests/requirements.pip
=== modified file 'tests/requirements.pip'
--- tests/requirements.pip 2013-11-07 11:17:54 +0000
+++ tests/requirements.pip 2013-11-07 17:27:20 +0000
@@ -24,7 +24,7 @@

  # GUI server -> juju deployer.
  bzr==2.6.0
-jujuclient==0.13.0
+jujuclient==0.13

  # Charm tests + juju deployer.
  websocket-client==0.12.0
@@ -34,7 +34,7 @@

  # GUI server.
  futures==2.1.4
--e bzr+http://launchpad.net/juju-deployer/darwin#egg=juju-deployer
+juju-deployer==0.2.8
  tornado==3.1.1

  # Charm hooks.