Merge lp:~hatch/juju-gui/better-bundle-error into lp:juju-gui/experimental

[Jeff Pihach]

Improve the error reporting of bundle imports

https://codereview.appspot.com/17540044/

[Jeff Pihach (hatch)]

Reviewers: mp+193455_code.launchpad.net,

Message:
Please take a look.

Description:
Improve the error reporting of bundle imports

https://code.launchpad.net/~hatch/juju-gui/better-bundle-error/+merge/193455

(do not edit description out of merge proposal)

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

Affected files (+20, -9 lines):
   A [revision details]
   M app/store/env/fakebackend.js
   M app/store/env/sandbox.js
   M test/test_fakebackend.js
   M test/test_sandbox_go.js
   M test/test_sandbox_python.js

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: test/test_fakebackend.js
=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js 2013-10-30 21:58:08 +0000
+++ test/test_fakebackend.js 2013-10-31 16:35:27 +0000
@@ -204,11 +204,13 @@
        // The service name is provided explicitly.
        fakebackend.deploy(
            'cs:precise/haproxy-18', callback, {name: 'wordpress'});
- assert.equal(result.error, 'A service with this name already
exists.');
+ assert.equal(result.error,
+ 'A service with this name already exists. (wordpress)');
        // The service name is derived from charm.
        result = undefined;
        fakebackend.deploy('cs:precise/wordpress-15', callback);
- assert.equal(result.error, 'A service with this name already
exists.');
+ assert.equal(result.error,
+ 'A service with this name already exists. (wordpress)');
      });

      it('reuses already-loaded charms with the same explicit id.',
function() {

Index: test/test_sandbox_go.js
=== modified file 'test/test_sandbox_go.js'
--- test/test_sandbox_go.js 2013-10-11 13:47:43 +0000
+++ test/test_sandbox_go.js 2013-10-31 16:35:27 +0000
@@ -333,7 +333,7 @@
        state.deploy('cs:precise/wordpress-15', function() {});
        var callback = function(result) {
          assert.equal(
- result.err, 'A service with this name already exists.');
+ result.err, 'A service with this name already exists.
(wordpress)');
          done();
        };
        env.deploy('cs:precise/wordpress-15', undefined, undefined,
undefined,

Index: test/test_sandbox_python.js
=== modified file 'test/test_sandbox_python.js'
--- test/test_sandbox_python.js 2013-09-26 22:03:11 +0000
+++ test/test_sandbox_python.js 2013-10-31 16:35:27 +0000
@@ -409,7 +409,8 @@
        env.after('defaultSeriesChange', function() {
          var callback = function(result) {
            assert.equal(
- result.err, 'A service with this name already exists.');
+ result.err,
+ 'A service with this name already exists. (wordpress)');
            done();
          };
          env.deploy(

Index: app/store/env/fakebackend.js
=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js 2013-10-30 21:58:08 +0000
+++ app/store/env/fakebackend.js 2013-10-31 16:35:27 +0000
...

[Gary Poster (gary)]

After adding the additional string changes I requested, this LGTM except
for the sandbox change. The fakebackend should be internally consistent
between error and Error. ISTM that error is consistently used as the
attr name. Where is Error coming from?

QAOK.

Thank you!

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode304
app/store/env/fakebackend.js:304: callbacks.failure({error: 'Invalid
charm id.'}));
Please include the charm id here.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode386
app/store/env/fakebackend.js:386: options.name + ')'});
Let's put it within the sentence:

A service with this name already exists (FOO).

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode400
app/store/env/fakebackend.js:400: console.log(options);
Maybe log the e too?

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode475
app/store/env/fakebackend.js:475: return {error: 'Invalid service id.'};
Please include serviceName in string

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode489
app/store/env/fakebackend.js:489: return {error: 'Error removing units:
' + result.error};
Maybe "Error removing units [array] of ${serviceName}: "

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode491
app/store/env/fakebackend.js:491: return {error: 'Warning removing
units: ' + result.warning};
...units [array] of ${serviceName}

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode513
app/store/env/fakebackend.js:513: return {error: 'Invalid service id.'};
include serviceName

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode581
app/store/env/fakebackend.js:581: return {error: 'Invalid number of
units.'};
Include serviceName and maybe numUnits

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode866
app/store/env/fakebackend.js:866: result = {error: 'Specified relation
is unavailable.'};
...eh, specifying the endpoints would be nice, but seems annoying. :-)

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode868
app/store/env/fakebackend.js:868: result = {error: 'Ambiguous
relationship is not allowed.'};
Same.

https://codereview.appspot.com/17540044/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/17540044/diff/1/app/store/env/sandbox.js#newcode1315
app/store/env/sandbox.js:1315: response.Error = reply.Error ||
reply.error;
Yeah, this is broken. We should not have to do that. The fakebackend
should be internally consistent. It looked like it was to me. What is
not consistent within fakebackend?

https://codereview.appspot.com/17540044/

[Jeff Pihach (hatch)]

Thanks for the review - comments and changes are included below.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode304
app/store/env/fakebackend.js:304: callbacks.failure({error: 'Invalid
charm id.'}));
On 2013/10/31 17:30:21, gary.poster wrote:
> Please include the charm id here.

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode386
app/store/env/fakebackend.js:386: options.name + ')'});
On 2013/10/31 17:30:21, gary.poster wrote:
> Let's put it within the sentence:

> A service with this name already exists (FOO).

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode400
app/store/env/fakebackend.js:400: console.log(options);
On 2013/10/31 17:30:21, gary.poster wrote:
> Maybe log the e too?

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode475
app/store/env/fakebackend.js:475: return {error: 'Invalid service id.'};
On 2013/10/31 17:30:21, gary.poster wrote:
> Please include serviceName in string

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode489
app/store/env/fakebackend.js:489: return {error: 'Error removing units:
' + result.error};
On 2013/10/31 17:30:21, gary.poster wrote:
> Maybe "Error removing units [array] of ${serviceName}: "

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode491
app/store/env/fakebackend.js:491: return {error: 'Warning removing
units: ' + result.warning};
On 2013/10/31 17:30:21, gary.poster wrote:
> ...units [array] of ${serviceName}

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode513
app/store/env/fakebackend.js:513: return {error: 'Invalid service id.'};
On 2013/10/31 17:30:21, gary.poster wrote:
> include serviceName

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode581
app/store/env/fakebackend.js:581: return {error: 'Invalid number of
units.'};
On 2013/10/31 17:30:21, gary.poster wrote:
> Include serviceName and maybe numUnits

Done.

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode866
app/store/env/fakebackend.js:866: result = {error: 'Specified relation
is unavailable.'};
:-) I added a console log of the endpoints so at least the data is
available to the user if necessary

https://codereview.appspot.com/17540044/diff/1/app/store/env/fakebackend.js#newcode868
app/store/env/fakebackend.js:868: result = {error: 'Ambiguous
relationship is not allowed.'};
Same

https://codereview.appspot.com/17540044/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/17540044/diff/1/app/store/env/sandbox.js#newcode1315
app/store/env/sandbox.js:1315: response.Error = reply.Error ||
reply.error;
the fakebackend uses .error as I would expect. You can see this with the
'statics' UNAUTHENTICATED_ERROR and VALUE_ERROR both use 'error' as
their key.

The Go env uses 'Error' as it's key probably to match the api respon...

[Jeff Pihach (hatch)]

*** Submitted:

Improve the error reporting of bundle imports

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

https://codereview.appspot.com/17540044/

=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js	2013-10-30 21:58:08 +0000
+++ app/store/env/fakebackend.js	2013-10-31 16:38:55 +0000
@@ -381,10 +381,13 @@
         options.name = charm.get('package_name');
       }
       if (this.db.services.getById(options.name)) {
-        return callback({error: 'A service with this name already exists.'});
+        console.log(options);
+        return callback({error: 'A service with this name already exists. (' +
+              options.name + ')'});
       }
       if (options.configYAML) {
         if (!Y.Lang.isString(options.configYAML)) {
+          console.log(options);
           return callback(
               {error: 'Developer error: configYAML is not a string.'});
         }
@@ -394,6 +397,7 @@
           options.config = jsyaml.safeLoad(options.configYAML);
         } catch (e) {
           if (e instanceof jsyaml.YAMLException) {
+            console.log(options);
             return callback({error: 'Error parsing YAML.\n' + e});
           }
           throw e;
@@ -1560,7 +1564,8 @@
             callback({DeploymentId: self._deploymentId});
           }, function(err) {
             deployStatus.Status = 'failed';
-            callback({Error: err.toString()});
+            console.log(err);
+            callback({Error: err.error});
           });
     },
 

=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js	2013-10-11 13:47:43 +0000
+++ app/store/env/sandbox.js	2013-10-31 16:38:55 +0000
@@ -1310,9 +1310,10 @@
             DeployerId: reply.DeploymentId
           }
         };
-        if (reply.Error) {
-          response.Error = reply.Error;
-        }
+        // Because the error can come in two different formats
+        // depending on the backend we need to check both.
+        response.Error = reply.Error || reply.error;
+
         client.receive(response);
       };
       state.importDeployer(data.Params.YAML, data.Params.Name,

=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js	2013-10-30 21:58:08 +0000
+++ test/test_fakebackend.js	2013-10-31 16:38:55 +0000
@@ -204,11 +204,13 @@
       // The service name is provided explicitly.
       fakebackend.deploy(
           'cs:precise/haproxy-18', callback, {name: 'wordpress'});
-      assert.equal(result.error, 'A service with this name already exists.');
+      assert.equal(result.error,
+          'A service with this name already exists. (wordpress)');
       // The service name is derived from charm.
       result = undefined;
       fakebackend.deploy('cs:precise/wordpress-15', callback);
-      assert.equal(result.error, 'A service with this name already exists.');
+      assert.equal(result.error,
+          'A service with this name already exists. (wordpress)');
     });
 
     it('reuses already-loaded charms with the same explicit id.', function() {

=== modified file 'test/test_sandbox_go.js'
--- test/test_sandbox_go.js	2013-10-11 13:47:43 +0000
+++ test/test_sandbox_go.js	2013-10-31 16:38:55 +0000
@@ -333,7 +333,7 @@
       state.deploy('cs:precise/wordpress-15', function() {});
       var callback = function(result) {
         assert.equal(
-            result.err, 'A service with this name already exists.');
+            result.err, 'A service with this name already exists. (wordpress)');
         done();
       };
       env.deploy('cs:precise/wordpress-15', undefined, undefined, undefined,

=== modified file 'test/test_sandbox_python.js'
--- test/test_sandbox_python.js	2013-09-26 22:03:11 +0000
+++ test/test_sandbox_python.js	2013-10-31 16:38:55 +0000
@@ -409,7 +409,8 @@
       env.after('defaultSeriesChange', function() {
         var callback = function(result) {
           assert.equal(
-              result.err, 'A service with this name already exists.');
+              result.err,
+              'A service with this name already exists. (wordpress)');
           done();
         };
         env.deploy(