Code review comment for lp:~bcsaller/juju-gui/deployer-export

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/importexport.js
File app/views/topology/importexport.js (right):

https://codereview.appspot.com/10563043/diff/1/app/views/topology/importexport.js#newcode112
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.

https://codereview.appspot.com/10563043/