Finish go-sandbox implementation

Finish go-sandbox implementation

This should be the rest of the methods needed to complete the Go sandbox implementation.  QA requested from both reviewers - I've been QAing through the day, but am proposing at EoD.

https://code.launchpad.net/~makyo/juju-gui/go-sandbox-methods/+merge/180017

(do not edit description out of merge proposal)

[matthew.scott, 2013-08-13 23:26:16]

Please take a look.

[matthew.scott, 2013-08-14 14:36:52]

Please take a look.

[benjamin.saller, 2013-08-14 16:32:12]

LGTM 

Thanks for this, all I had were some minor suggestions, but take it or leave it,
good work.

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

https://codereview.appspot.com/12900043/diff/4001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:980: },
This is just a suggestion. I like the clean up and usage below but sometimes it
reads as

_basicReceive(client, requestFunction(), ...)

 where the request is a little hidden in the method call. It might be a nit-pic
but if you agree it could be clearer I'd like to see the request broken out to
the line before in each case.

https://codereview.appspot.com/12900043/diff/4001/test/test_sandbox_go.js
File test/test_sandbox_go.js (right):

https://codereview.appspot.com/12900043/diff/4001/test/test_sandbox_go.js#new...
test/test_sandbox_go.js:295: it('can destroy', function(done) {
it('can destroy a service'

I didn't see that this was nested in a describe related to only services so I'd
qualify it.

[matthew.scott, 2013-08-14 17:16:57]

Thanks for the review!

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

https://codereview.appspot.com/12900043/diff/4001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:980: },
On 2013/08/14 16:32:12, benjamin.saller wrote:
> This is just a suggestion. I like the clean up and usage below but sometimes
it
> reads as
> 
> _basicReceive(client, requestFunction(), ...)
> 
>  where the request is a little hidden in the method call. It might be a
nit-pic
> but if you agree it could be clearer I'd like to see the request broken out to
> the line before in each case.

Done.

https://codereview.appspot.com/12900043/diff/4001/test/test_sandbox_go.js
File test/test_sandbox_go.js (right):

https://codereview.appspot.com/12900043/diff/4001/test/test_sandbox_go.js#new...
test/test_sandbox_go.js:295: it('can destroy', function(done) {
On 2013/08/14 16:32:12, benjamin.saller wrote:
> it('can destroy a service'
> 
> I didn't see that this was nested in a describe related to only services so
I'd
> qualify it.

Done.

[matthew.scott, 2013-08-14 17:19:33]

Please take a look.

[rharding, 2013-08-14 17:42:15]

LGTM 

Some minor questions to ask. I did notice all the tests were "this can do this"
while some of the code was around handling error conditions and those went
through the basicReceive method vs the normal response. Is it important to
verify the error paths?

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

https://codereview.appspot.com/12900043/diff/4001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:965: Receive a basic response. Several API calls simply
return a request ID
indent comments in two spaces?

https://codereview.appspot.com/12900043/diff/4001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:975: var response = {RequestId: request.RequestId,
Response: {}};
please newline these as the R's run together across the line.

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

https://codereview.appspot.com/12900043/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:974: _basicReceive: function(client, request, result) {
the functions using this seem to usually be data, client, state. This call has
client first. There's even a data call down in line 1113, should request/client
swap?

https://codereview.appspot.com/12900043/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:996: state.deploy(data.Params.CharmUrl, callback, {
can we bind the callback directly and not need the self reference?

https://codereview.appspot.com/12900043/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:1303: 'timers'
<3

[matthew.scott, 2013-08-14 18:54:45]

Thanks for the review.  The error path is tested just above in 'can communicate
errors after attempting to deploy', but I've added an additional test for
CharmInfo, which only uses the basicReceive method in the instance of an error.

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

https://codereview.appspot.com/12900043/diff/4001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:965: Receive a basic response. Several API calls simply
return a request ID
On 2013/08/14 17:42:15, rharding wrote:
> indent comments in two spaces?

I was maintaining consistency within the file; going to defer until I can do the
whole file in one slack task.

https://codereview.appspot.com/12900043/diff/4001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:975: var response = {RequestId: request.RequestId,
Response: {}};
On 2013/08/14 17:42:15, rharding wrote:
> please newline these as the R's run together across the line.

Done.

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

https://codereview.appspot.com/12900043/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:974: _basicReceive: function(client, request, result) {
On 2013/08/14 17:42:15, rharding wrote:
> the functions using this seem to usually be data, client, state. This call has
> client first. There's even a data call down in line 1113, should
request/client
> swap?

Done.

https://codereview.appspot.com/12900043/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:996: state.deploy(data.Params.CharmUrl, callback, {
On 2013/08/14 17:42:15, rharding wrote:
> can we bind the callback directly and not need the self reference?

Done.

[matthew.scott, 2013-08-14 19:31:01]

*** Submitted:

Finish go-sandbox implementation

This should be the rest of the methods needed to complete the Go sandbox
implementation.  QA requested from both reviewers - I've been QAing through the
day, but am proposing at EoD.

R=benjamin.saller, rharding
CC=
https://codereview.appspot.com/12900043