Add the CharmInfo API call to the go env.

Add the CharmInfo API call to the go env.



https://code.launchpad.net/~frankban/juju-gui/bug-1152340-charminfo/+merge/155497

(do not edit description out of merge proposal)

[frankban, 2013-03-26 14:13:19]

Please take a look.

[jeff.pihach, 2013-03-26 14:26:36]

LGTM Thanks! A few trivial comments and just curious if some of these tests
should be async.

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

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode617
app/store/env/go.js:617: get_charm: function(charmURL, callback) {
can we camelCase this?

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode627
app/store/env/go.js:627: }, intermediateCallback);
if callback is falsy then intermediateCallback will be undefined - was this
intentional? Are we checking for that possibility in _send_rpc() ?

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

https://codereview.appspot.com/7956045/diff/1/app/store/env/python.js#newcode203
app/store/env/python.js:203: get_charm: function(charmURL, callback) {
camelCase?

https://codereview.appspot.com/7956045/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/7956045/diff/1/test/test_env_go.js#newcode752
test/test_env_go.js:752: conn.msg({
This is async in another test - does it need to be here as well?

https://codereview.appspot.com/7956045/diff/1/test/test_env_go.js#newcode828
test/test_env_go.js:828: env.get_charm('cs:precise/wordpress-10', function(data)
{
do these calls to getCharm need to use async and the done() mocha method?

[benji, 2013-03-26 15:34:45]

LGTM

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

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode28
app/store/env/go.js:28: var newObj = {};
It is probably not important here, but a bit of trivia for you: If you want to
create a truly empty object (without all the defaults that {} includes) you can
do so like this: Object.create(null).

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode627
app/store/env/go.js:627: }, intermediateCallback);
On 2013/03/26 14:26:36, jeff.pihach wrote:
> if callback is falsy then intermediateCallback will be undefined - was this
> intentional? Are we checking for that possibility in _send_rpc() ?

Yep, the callback is optional, so _send_rpc is fine with that eventuality.  We
might want to add a comment that spells out the effect here though.

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

https://codereview.appspot.com/7956045/diff/1/app/store/env/python.js#newcode204
app/store/env/python.js:204: this._send_rpc({'op': 'get_charm', 'charm_url':
charmURL}, callback);
We can elide the quotes around the keys if you like.

[frankban, 2013-03-26 17:18:51]

Thanks for the reviews Jeff and Benji.

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

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode28
app/store/env/go.js:28: var newObj = {};
On 2013/03/26 15:34:45, benji wrote:
> It is probably not important here, but a bit of trivia for you: If you want to
> create a truly empty object (without all the defaults that {} includes) you
can
> do so like this: Object.create(null).

Cool! Changed the code to use that.

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode617
app/store/env/go.js:617: get_charm: function(charmURL, callback) {
On 2013/03/26 14:26:36, jeff.pihach wrote:
> can we camelCase this?

You mean, get_charm? Unfortunately we can't. We have to be compatible with the
legacy env.

https://codereview.appspot.com/7956045/diff/1/app/store/env/go.js#newcode627
app/store/env/go.js:627: }, intermediateCallback);
On 2013/03/26 15:34:45, benji wrote:
> On 2013/03/26 14:26:36, jeff.pihach wrote:
> > if callback is falsy then intermediateCallback will be undefined - was this
> > intentional? Are we checking for that possibility in _send_rpc() ?
> 
> Yep, the callback is optional, so _send_rpc is fine with that eventuality.  We
> might want to add a comment that spells out the effect here though.

Done.

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

https://codereview.appspot.com/7956045/diff/1/app/store/env/python.js#newcode204
app/store/env/python.js:204: this._send_rpc({'op': 'get_charm', 'charm_url':
charmURL}, callback);
On 2013/03/26 15:34:45, benji wrote:
> We can elide the quotes around the keys if you like.

Done.

https://codereview.appspot.com/7956045/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/7956045/diff/1/test/test_env_go.js#newcode752
test/test_env_go.js:752: conn.msg({
On 2013/03/26 14:26:36, jeff.pihach wrote:
> This is async in another test - does it need to be here as well?

This involves firing and receiving the 'msg' event, published by the
environment. Not sure, but maybe it's better to take advantage of done() here.
Fixed (also in the "handle failures") test below.

https://codereview.appspot.com/7956045/diff/1/test/test_env_go.js#newcode828
test/test_env_go.js:828: env.get_charm('cs:precise/wordpress-10', function(data)
{
On 2013/03/26 14:26:36, jeff.pihach wrote:
> do these calls to getCharm need to use async and the done() mocha method?

Since the environment uses the socket stub, the call to env.get_charm and the
subsequent call to env._send_rpc are not async, AFAIK.

[frankban, 2013-03-26 17:30:43]

*** Submitted:

Add the CharmInfo API call to the go env.

R=jeff.pihach, benji
CC=
https://codereview.appspot.com/7956045