Exports in Deployer format.

Exports in Deployer format.

Switch default hotkey export to YAML based deployer format. It we don't import
this format yet.

https://code.launchpad.net/~bcsaller/juju-gui/deployer-export/+merge/171402

(do not edit description out of merge proposal)

[bcsaller, 2013-06-25 19:47:33]

Please take a look.

[jeff.pihach, 2013-06-25 20:08:12]

LGTM !

https://codereview.appspot.com/10563043/diff/1/app/views/topology/importexpor...
File app/views/topology/importexport.js (right):

https://codereview.appspot.com/10563043/diff/1/app/views/topology/importexpor...
app/views/topology/importexport.js:112: var self = this;
unused

[benji, 2013-06-25 20:37:03]

This branch looks good.  I had a few polish suggestions.

LGTM-ly y'rs,
Benji

https://codereview.appspot.com/10563043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10563043/diff/1/app/app.js#newcode239
app/app.js:239: var result = this.db.exportDeployer();
A small thought: "exportDeployer" (despite it's lack of capitalization and
having no "new") read like a constructor to me.  Perhaps a more "verby" name
would be an improvement.  Maybe just "export" since we have only one export
format (right?).

https://codereview.appspot.com/10563043/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode183
app/models/models.js:183: }
It looks like an empty string would work as well as undefined, so lines 179-182
could be replaced with just "return result.join(',');".

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode849
app/models/models.js:849: * @param {Object} db for application.
This was probably left from an earlier iteration.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode855
app/models/models.js:855: relationList = this.relations,
This is not a request for a change, more of adding a perspective:  I find these
sorts of shallow aliases make code harder to reason about because they add a
level of indirection that I have to track down and keep in my head when reading
the code.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode857
app/models/models.js:857: envExport: {
It seems to me that we don't need the "envExport" layer in here.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode867
app/models/models.js:867: if (s.get('pending') === true) {return;}
"=== true" makes me twitch. ;)  Would "if (s.get('pending')) ..." work instead?

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode871
app/models/models.js:871: // path in the future.
This was a helpful comment.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode874
app/models/models.js:874: num_units: units && units.size() || 1
A comment about the circumstances in which s.units is not defined might be nice.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode884
app/models/models.js:884: relationList.each(function(r) {
Single character variable names... have you been working on juju-core?  I kid, I
kid.  It might be nice to make that "relation" and the "s" above "service",
though.

https://codereview.appspot.com/10563043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/10563043/diff/1/test/test_model.js#newcode814
test/test_model.js:814: 
It feels like there should be more testing.  For example: is YAML actually
produced, is the "missing units" corner case properly handled, etc.

[benjamin.saller, 2013-06-25 20:50:31]

Thanks for the reviews. Made most of the minor updates indicated. Will submit
soon.

https://codereview.appspot.com/10563043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10563043/diff/1/app/app.js#newcode239
app/app.js:239: var result = this.db.exportDeployer();
On 2013/06/25 20:37:03, benji wrote:
> A small thought: "exportDeployer" (despite it's lack of capitalization and
> having no "new") read like a constructor to me.  Perhaps a more "verby" name
> would be an improvement.  Maybe just "export" since we have only one export
> format (right?).

export is a reserved word in JS and makes linters unhappy. This does return an
object but its not really a constuctor as that objects prototype is the default.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode183
app/models/models.js:183: }
On 2013/06/25 20:37:03, benji wrote:
> It looks like an empty string would work as well as undefined, so lines
179-182
> could be replaced with just "return result.join(',');".

It would but I would rather not return a string if we don't expect to parse it
(comma delimited). If you don't feel strongly I'll leave this for now.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode849
app/models/models.js:849: * @param {Object} db for application.
On 2013/06/25 20:37:03, benji wrote:
> This was probably left from an earlier iteration.

Nice catch and exactly right.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode857
app/models/models.js:857: envExport: {
On 2013/06/25 20:37:03, benji wrote:
> It seems to me that we don't need the "envExport" layer in here.

The deployer format labels each section within the YAML. For example this might
be 'blog' and there could be another block called 'productionBlog' which
inherits blog and so on. We don't deal with inheritance as we don't retain any
of the initial structure (and in fact don't import these things at all yet). 

This is provided as a default name to match the format.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode874
app/models/models.js:874: num_units: units && units.size() || 1
On 2013/06/25 20:37:03, benji wrote:
> A comment about the circumstances in which s.units is not defined might be
nice.

Testing or ghosts. 'units' is created on the delta event under each service now
automatically. Added to code as well.

https://codereview.appspot.com/10563043/diff/1/app/models/models.js#newcode884
app/models/models.js:884: relationList.each(function(r) {
On 2013/06/25 20:37:03, benji wrote:
> Single character variable names... have you been working on juju-core?  I kid,
I
> kid.  It might be nice to make that "relation" and the "s" above "service",
> though.

Done.

https://codereview.appspot.com/10563043/diff/1/app/views/topology/importexpor...
File app/views/topology/importexport.js (right):

https://codereview.appspot.com/10563043/diff/1/app/views/topology/importexpor...
app/views/topology/importexport.js:112: var self = this;
On 2013/06/25 20:08:12, jeff.pihach wrote:
> unused

Done.

https://codereview.appspot.com/10563043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/10563043/diff/1/test/test_model.js#newcode814
test/test_model.js:814: 
On 2013/06/25 20:37:03, benji wrote:
> It feels like there should be more testing.  For example: is YAML actually
> produced, is the "missing units" corner case properly handled, etc.

I suspect you are correct, however there is still a chance the deployer side of
the format might need some changes so I didn't nail it down that tightly. 

I'm more concerned that the object is correct though than the YAML output at
this phase as I sort of have to trust the 3rd party serializer.

[bcsaller, 2013-06-25 20:57:34]

*** Submitted:

Exports in Deployer format.

Switch default hotkey export to YAML based deployer format. It we don't import
this format yet.

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