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.