Improve the error reporting of bundle imports

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)

[jeff.pihach, 2013-10-31 16:39:13]

Please take a look.

[gary.poster, 2013-10-31 17:30:21]

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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#newco...
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?

[jeff.pihach, 2013-10-31 18:01:40]

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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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#newco...
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 response.

[jeff.pihach, 2013-10-31 18:54:10]

*** Submitted:

Improve the error reporting of bundle imports

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