Import Deployer in Golang env

Import Deployer in Golang env

This include fakebackend support, and actual go env support. DnD import
has been changed to use the env call rather than doing a direct import.

This also includes support for Global import status (but not individual 
deployment watches which shouldn't be needed).

(The initial submit of this failed before unnoticed)

https://code.launchpad.net/~bcsaller/juju-gui/importGoEnv/+merge/185398

(do not edit description out of merge proposal)

[bcsaller, 2013-09-13 00:26:46]

Please take a look.

[jeff.pihach, 2013-09-13 16:30:44]

This looks good!

QA Failed (Chrome Ubuntu)

When dragging and dropping the wp-deployer.yaml file:
There is the notification that it was imported successfully but then nothing
shows up on the canvas for a few seconds (likely requires the delta to come
back)
Uncaught TypeError: Cannot read property 'binlog-format' of undefined:
sandbox.js:1278
Uncaught TypeError: Cannot read property 'debug' of undefined: sandbox.js:1278

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1237: the last 5 imports. We don't currently queue
but this a
s/this//

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/go.js#newcode418
app/store/env/go.js:418: deployerImport: function(yamlData, bundleName,
callback) {
To make bundleName truely optional you could add....

if (Y.Lang.isFunction(bundleName) {
  callback = bundleName;
}

Then you won't need to pass 'null' in the core use case.

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1216: var self = this;
unused

https://codereview.appspot.com/13253050/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13253050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:711: env.deployerImport(e.target.result, null,
function(result) {
Will this not fail on PyJuju? Should there not be 

if (!Y.Lang.isFunction(env.deployerImport)) {
 // notify and return
}

https://codereview.appspot.com/13253050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:723: level: 'error'
I'd add a console.log here which logs the actual objects

https://codereview.appspot.com/13253050/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13253050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:550: assert.equal(fakebackend.db.relations.size(), 1);
This and the assert expecting 1 above should have a failure comment so that it's
clear which one is failing.

https://codereview.appspot.com/13253050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:565: assert.equal(status.LastChanges[0].DeploymentId,
1);
failure comment as above

https://codereview.appspot.com/13253050/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/13253050/diff/1/test/utils.js#newcode141
test/utils.js:141: var modelList = cache;
modelList is now unused.

[benji, 2013-09-13 17:04:49]

LGTM

Thanks for the branch it looks good.  I found a few small things that could be
improved.

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1196: @param {String} YAMLData to import.
This style of yuidoc, which reads well in the source does not work well when
rendered.  For example, the above renders to this:

    * YAMLData String
      to import.

Therefore, we need to use complete (ish) sentences for the prose parts of the
yuidoc.

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1197: @param {String} name (optional) of bundle to
import.
The yuidoc way of specifying optionality is by placing square braces around the
parameter name:

    @param {String} [name] The name of the bundle to import.

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1198: @param {Function} callback to trigger.
An "@return {undefined} Nothing." would be nice.  There are several places where
they could be added, I'll only mention this one.

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1227: self._importChanges.length - 5);
slice can take negative indices, so this would work too:

self._importChanges = self._importChanges.slice(-5);

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/go.js#newcode414
app/store/env/go.js:414: * @param {String} bundleName optional.
More yuidoc syntax: [bundleName]

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js
File test/test_sandbox_go.js (right):

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:1164: it('can import deployer files (integration).',
function(done) {
I don't understand what the parenthetical note signifies here.  Maybe an
explanatory comment in the body of the test would help.

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:1177: it('should support deployer status (w/import)',
function(done) {
I can't correlate this test description with what the test does.

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:1203: it('should support deployer status (empty)',
function(done) {
Oh, now I think I'm starting to understand the words in parenthesis.  Comments
would help here.

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:1222: it('can import deployer status (integration).',
function(done) {
More parenthesis I don't understand.

[bcsaller, 2013-09-13 18:07:21]

Please take a look.

[benjamin.saller, 2013-09-13 18:09:55]

Thanks for the reviews, changes pushed. Should pass QA now

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1197: @param {String} name (optional) of bundle to
import.
On 2013/09/13 17:04:50, benji wrote:
> The yuidoc way of specifying optionality is by placing square braces around
the
> parameter name:
> 
>     @param {String} [name] The name of the bundle to import.
> 	

Thanks for these YUI doc hints, I never look at the output so I didn't make
proper room in my head for this till now.

https://codereview.appspot.com/13253050/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1227: self._importChanges.length - 5);
On 2013/09/13 17:04:50, benji wrote:
> slice can take negative indices, so this would work too:
> 
> self._importChanges = self._importChanges.slice(-5);

Done.

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/go.js#newcode418
app/store/env/go.js:418: deployerImport: function(yamlData, bundleName,
callback) {
On 2013/09/13 16:30:44, jeff.pihach wrote:
> To make bundleName truely optional you could add....
> 
> if (Y.Lang.isFunction(bundleName) {
>   callback = bundleName;
> }
> 
> Then you won't need to pass 'null' in the core use case.

I added a version of this, thanks.

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

https://codereview.appspot.com/13253050/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1216: var self = this;
On 2013/09/13 16:30:44, jeff.pihach wrote:
> unused

Done.

https://codereview.appspot.com/13253050/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13253050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:711: env.deployerImport(e.target.result, null,
function(result) {
On 2013/09/13 16:30:44, jeff.pihach wrote:
> Will this not fail on PyJuju? Should there not be 
> 
> if (!Y.Lang.isFunction(env.deployerImport)) {
>  // notify and return
> }

Sounds like a good use for our 'implements' call ;)

But yes, this won't work on PyJuju now, I can put a guard around it, but we will
also be changing the interface around this to be a button down the road I think.

https://codereview.appspot.com/13253050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:723: level: 'error'
On 2013/09/13 16:30:44, jeff.pihach wrote:
> I'd add a console.log here which logs the actual objects 

Done.

https://codereview.appspot.com/13253050/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13253050/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:550: assert.equal(fakebackend.db.relations.size(), 1);
On 2013/09/13 16:30:44, jeff.pihach wrote:
> This and the assert expecting 1 above should have a failure comment so that
it's
> clear which one is failing.

Done.

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js
File test/test_sandbox_go.js (right):

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:1164: it('can import deployer files (integration).',
function(done) {
On 2013/09/13 17:04:50, benji wrote:
> I don't understand what the parenthetical note signifies here.  Maybe an
> explanatory comment in the body of the test would help.

This is a pattern used in this file for a number of tests. It basically says do
a test simulating a message on the wire and then do a test (integration) using
the real env method. This helps us ensure that we are pushing the proper calls
on the wire and mapping data back as expected.

https://codereview.appspot.com/13253050/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:1203: it('should support deployer status (empty)',
function(done) {
On 2013/09/13 17:04:50, benji wrote:
> Oh, now I think I'm starting to understand the words in parenthesis.  Comments
> would help here.

Here it is outside the above pattern and will be changed to be
part of the spec sentence.

[bcsaller, 2013-09-16 15:47:09]

Please take a look.

[jeff.pihach, 2013-09-16 15:52:27]

Looks good, thanks for sticking with it!

QA OK

[jeff.pihach, 2013-09-16 15:53:23]

LGTM

[bcsaller, 2013-09-18 18:21:51]

*** Submitted:

Import Deployer in Golang env

This include fakebackend support, and actual go env support. DnD import
has been changed to use the env call rather than doing a direct import.

This also includes support for Global import status (but not individual 
deployment watches which shouldn't be needed).

(The initial submit of this failed before unnoticed)

R=jeff.pihach, benji, benjamin.saller
CC=
https://codereview.appspot.com/13253050