Merge lp:~bac/charms/precise/juju-gui/enverror into lp:~juju-gui/charms/precise/juju-gui/trunk

[Brad Crittenden]

Extract the message from EnvError on deploy err.

https://codereview.appspot.com/34270044/

[Brad Crittenden (bac)]

Reviewers: mp+196966_code.launchpad.net,

Message:
Please take a look.

Description:
Extract the message from EnvError on deploy err.

https://code.launchpad.net/~bac/charms/precise/juju-gui/enverror/+merge/196966

(do not edit description out of merge proposal)

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

Affected files (+21, -5 lines):
   A [revision details]
   M revision
   M server/guiserver/bundles/utils.py
   M server/guiserver/tests/bundles/test_base.py
   M server/guiserver/tests/bundles/test_utils.py

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-14 11:25:13 +0000
+++ revision 2013-11-27 19:26:26 +0000
@@ -1,1 +1,1 @@
-100
+101

Index: server/guiserver/bundles/utils.py
=== modified file 'server/guiserver/bundles/utils.py'
--- server/guiserver/bundles/utils.py 2013-11-19 14:56:29 +0000
+++ server/guiserver/bundles/utils.py 2013-11-27 19:25:54 +0000
@@ -30,7 +30,7 @@
  from tornado.httpclient import AsyncHTTPClient

  from guiserver.watchers import AsyncWatcher
-
+from jujuclient import EnvError

  # Change statuses.
  SCHEDULED = 'scheduled'
@@ -73,7 +73,10 @@
      """
      logging.error('error deploying the bundle')
      logging.error('error type: {}'.format(type(exception)))
- message = str(exception).strip()
+ if isinstance(exception, EnvError):
+ message = exception.message.strip()
+ else:
+ message = str(exception).strip()
      if message:
          logging.error('error message: {}'.format(message))
      else:

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-19 15:28:56 +0000
+++ server/guiserver/tests/bundles/test_base.py 2013-11-27 19:25:54 +0000
@@ -250,7 +250,7 @@
          expected = {
              'DeploymentId': 0,
              'Status': utils.COMPLETED,
- 'Error': "<Env Error - Details:\n { 'Error': 'bad wolf'}\n
>",
+ 'Error': "bad wolf",
              'Time': 42,
          }
          self.assertEqual(expected, status[0])

Index: server/guiserver/tests/bundles/test_utils.py
=== modified file 'server/guiserver/tests/bundles/test_utils.py'
--- server/guiserver/tests/bundles/test_utils.py 2013-11-19 15:28:56 +0000
+++ server/guiserver/tests/bundles/test_utils.py 2013-11-27 19:28:03 +0000
@@ -32,7 +32,7 @@
  from guiserver import watchers
  from guiserver.bundles import utils
  from guiserver.tests import helpers
-
+from jujuclient import EnvError

  mock_time = mock.patch('time.time', mock.Mock(return_value=12345))

@@ -93,6 +93,17 @@
                  error = utils.message_from_error(ValueError('bad wolf'))
          self.assertEqual('bad wolf', error)

+ def test_env_error_extracted(self):
+ # An EnvError as returned from the Go environment is not suitable
for
+ # display to t...

[Brad Crittenden (bac)]

To QA:

The charm needs to have the juju-gui trunk. I tried just setting
juju-gui-source but ran into all kinds of problems.
What I eventually did was make a new release in juju-gui and copy it
over to my charm '/releases' directory. That's what I describe here.

In juju-gui:
BRANCH_IS_GOOD=1 make distfile
Note the new tar ball created.

0) Grab the branch: lp:~bac/charms/precise/juju-gui/enverror
0.1) Copy tarball from juju-gui to the charm releases directory.
1) juju switch local
2) sudo juju bootstrap
3) make deploy

Now go to the GUI. Search for 'owner: bac'. Drag bac's Jenkins bundle
onto the canvas. After a short while you should get a notification
which says:

Updated status for deployment id: 0

The deployment errored: json: cannot unmarshal string into Go value of
type uint64

https://codereview.appspot.com/34270044/diff/1/server/guiserver/tests/bundles/test_base.py
File server/guiserver/tests/bundles/test_base.py (right):

https://codereview.appspot.com/34270044/diff/1/server/guiserver/tests/bundles/test_base.py#newcode253
server/guiserver/tests/bundles/test_base.py:253: 'Error': "bad wolf",
The returned error is now more concise.

https://codereview.appspot.com/34270044/

[Gary Poster (gary)]

LGTM and QA good. Very nice that the fix could be so simple.

(Separately, I wish the errors were easier to spot, but that's a
different problem.)

Thank you!

https://codereview.appspot.com/34270044/

[Brad Crittenden (bac)]

*** Submitted:

Extract the message from EnvError on deploy err.

R=gary.poster
CC=
https://codereview.appspot.com/34270044

https://codereview.appspot.com/34270044/

=== modified file 'revision'
--- revision	2013-11-14 11:25:13 +0000
+++ revision	2013-11-27 19:32:10 +0000
@@ -1,1 +1,1 @@
-100
+101

=== modified file 'server/guiserver/bundles/utils.py'
--- server/guiserver/bundles/utils.py	2013-11-19 14:56:29 +0000
+++ server/guiserver/bundles/utils.py	2013-11-27 19:32:10 +0000
@@ -30,7 +30,7 @@
 from tornado.httpclient import AsyncHTTPClient
 
 from guiserver.watchers import AsyncWatcher
-
+from jujuclient import EnvError
 
 # Change statuses.
 SCHEDULED = 'scheduled'
@@ -73,7 +73,10 @@
     """
     logging.error('error deploying the bundle')
     logging.error('error type: {}'.format(type(exception)))
-    message = str(exception).strip()
+    if isinstance(exception, EnvError):
+        message = exception.message.strip()
+    else:
+        message = str(exception).strip()
     if message:
         logging.error('error message: {}'.format(message))
     else:

=== modified file 'server/guiserver/tests/bundles/test_base.py'
--- server/guiserver/tests/bundles/test_base.py	2013-11-19 15:28:56 +0000
+++ server/guiserver/tests/bundles/test_base.py	2013-11-27 19:32:10 +0000
@@ -250,7 +250,7 @@
         expected = {
             'DeploymentId': 0,
             'Status': utils.COMPLETED,
-            'Error': "<Env Error - Details:\n {   'Error': 'bad wolf'}\n >",
+            'Error': "bad wolf",
             'Time': 42,
         }
         self.assertEqual(expected, status[0])

=== modified file 'server/guiserver/tests/bundles/test_utils.py'
--- server/guiserver/tests/bundles/test_utils.py	2013-11-19 15:28:56 +0000
+++ server/guiserver/tests/bundles/test_utils.py	2013-11-27 19:32:10 +0000
@@ -32,7 +32,7 @@
 from guiserver import watchers
 from guiserver.bundles import utils
 from guiserver.tests import helpers
-
+from jujuclient import EnvError
 
 mock_time = mock.patch('time.time', mock.Mock(return_value=12345))
 
@@ -93,6 +93,17 @@
                 error = utils.message_from_error(ValueError('bad wolf'))
         self.assertEqual('bad wolf', error)
 
+    def test_env_error_extracted(self):
+        # An EnvError as returned from the Go environment is not suitable for
+        # display to the user.  The Error field is extracted and returned.
+        expected_type = "error type: <class 'jujuclient.EnvError'>"
+        expected_message = 'error message: cannot parse json'
+        with ExpectLog('', expected_type, required=True):
+            with ExpectLog('', expected_message, required=True):
+                exception = EnvError({'Error': 'cannot parse json'})
+                error = utils.message_from_error(exception)
+        self.assertEqual('cannot parse json', error)
+
     def test_without_message(self):
         # A placeholder message is returned.
         expected_type = "error type: <type 'exceptions.SystemExit'>"