The beginings of a Go-flavored sandbox.

The beginings of a Go-flavored sandbox.

Logging in, deploying a service, and moving it around all work.

https://code.launchpad.net/~benji/juju-gui/go-sandbox/+merge/169036

(do not edit description out of merge proposal)

[benji, 2013-06-12 19:06:00]

Please take a look.

[gary.poster, 2013-06-12 19:35:35]

LGTM with questions

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

https://codereview.appspot.com/10235045/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:812: client.receive({Response: {AllWatcherId: '42'}});
Heh. :-)

https://codereview.appspot.com/10235045/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:839: var response = {RequestId: data.RequestId};
I guess we make a new response because Juju Core does not send back the whole
original request the way PyJuju does, right?  If so, cool...though I wonder if
we should therefore remove the copying code in line 766 ("Y.merge(data)" and the
preceding comment).

https://codereview.appspot.com/10235045/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:879: // We do not provide the optional Config or
Constraints in the response.
How do you get it then?  Or do you mean that this will be in a future branch? 
(If so, let's use TODO as you did elsewhere)

[benji, 2013-06-12 19:50:01]

Thanks for the review.  I've the things you pointed out.

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

https://codereview.appspot.com/10235045/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:839: var response = {RequestId: data.RequestId};
On 2013/06/12 19:35:35, gary.poster wrote:
> I guess we make a new response because Juju Core does not send back the whole
> original request the way PyJuju does, right?

Yep.  Core uses a more standard args/results pattern.

> If so, cool...though I wonder if
> we should therefore remove the copying code in line 766 ("Y.merge(data)" and
the
> preceding comment).

Good idea.  Done.

https://codereview.appspot.com/10235045/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:879: // We do not provide the optional Config or
Constraints in the response.
On 2013/06/12 19:35:35, gary.poster wrote:
> How do you get it then?  Or do you mean that this will be in a future branch? 
> (If so, let's use TODO as you did elsewhere)

Yep, missing TODO.  Fixed.

[benji, 2013-06-12 20:04:52]

Please take a look.

[jeff.pihach, 2013-06-12 21:16:12]

LGTM Thanks this looks good. I am not sure about the method casing senario :-)

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

https://codereview.appspot.com/10235045/diff/5001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:774: @method handle_Admin_Login
camel_Snake_Case? ;-)

https://codereview.appspot.com/10235045/diff/5001/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/10235045/diff/5001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1486: this.timeout(2000); // Long timeouts make async
failures hard to detect.
this technique has caused false failures on our CI before because of a slow
instance so I would suggest increasing it or removing all together.

https://codereview.appspot.com/10235045/diff/5001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1506: Y.each(cleanups, function(f) {f();});
cleanups is never used.

[benji, 2013-06-13 12:21:56]

Thanks for the good review Jeff.  I've fixed all the things you pointed out.

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

https://codereview.appspot.com/10235045/diff/5001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:774: @method handle_Admin_Login
On 2013/06/12 21:16:12, jeff.pihach wrote:
> camel_Snake_Case? ;-)

A good point succinctly made.

This came about from following the style of the Python sandbox, but I really see
no reason to preserve it.  I've changed these to be pure camel case.

The code word is :%s/handle_\(.*\)_\([^:(]*\)/handle\1\2/g

https://codereview.appspot.com/10235045/diff/5001/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/10235045/diff/5001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1486: this.timeout(2000); // Long timeouts make async
failures hard to detect.
On 2013/06/12 21:16:12, jeff.pihach wrote:
> this technique has caused false failures on our CI before because of a slow
> instance so I would suggest increasing it or removing all together. 

Sounds good.

These are an artifact of me basing the Go sandbox tests off of the Python
sandbox tests, so I will remove the timeouts in the originals as well.

https://codereview.appspot.com/10235045/diff/5001/test/test_sandbox.js#newcod...
test/test_sandbox.js:1506: Y.each(cleanups, function(f) {f();});
On 2013/06/12 21:16:12, jeff.pihach wrote:
> cleanups is never used.

Good catch.  Another artifact.  Fixed.

[benji, 2013-06-13 12:43:09]

Please take a look.