Merge lp:~frankban/charms/precise/juju-gui/improve-error-handling into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

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://codereview.appspot.com/23000043/

[Francesco Banconi (frankban)]

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

 ...

[Richard Harding (rharding)]

LGTM

https://codereview.appspot.com/23000043/

[Brad Crittenden (bac)]

LGTM, QA-A-OK

https://codereview.appspot.com/23000043/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/23000043/diff/1/hooks/utils.py#newcode124
hooks/utils.py:124: # lp:~frankban/python-jujuclient/pickable-enverror.
nice thanks for the branch reference.

https://codereview.appspot.com/23000043/diff/1/server/guiserver/handlers.py
File server/guiserver/handlers.py (right):

https://codereview.appspot.com/23000043/diff/1/server/guiserver/handlers.py#newcode232
server/guiserver/handlers.py:232: self.write(info)
Excellent

https://codereview.appspot.com/23000043/

[Francesco Banconi (frankban)]

*** Submitted:

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.

R=rharding, bac
CC=
https://codereview.appspot.com/23000043

https://codereview.appspot.com/23000043/

=== 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 18:19:14 +0000 differ
=== modified file 'hooks/utils.py'
--- hooks/utils.py	2013-11-07 14:41:27 +0000
+++ hooks/utils.py	2013-11-07 18:19:14 +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.
 
=== modified file 'revision'
--- revision	2013-11-07 11:18:20 +0000
+++ revision	2013-11-07 18:19:14 +0000
@@ -1,1 +1,1 @@
-93
+94
 
=== 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:19:14 +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):
 
=== 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 18:19:14 +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()
 
=== 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:19:14 +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)
 
 
=== modified file 'tests/requirements.pip'
--- tests/requirements.pip	2013-11-07 11:17:54 +0000
+++ tests/requirements.pip	2013-11-07 18:19:14 +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.