Merge lp://staging/~bcsaller/juju-gui/export-improvements into lp://staging/juju-gui/experimental

Reviewers: mp+174485_code.launchpad.net,

Message:
Please take a look.

Description:
Many deployer export format fixes

Export Annotations
Skip export of default service options.
Services is a dict, not an array
Use qualified charm names

https://code.launchpad.net/~bcsaller/juju-gui/export-improvements/+merge/174485

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11227043/

Affected files:
   A [revision details]
   M app/models/models.js
   M test/test_model.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test/test_model.js
=== modified file 'test/test_model.js'
--- test/test_model.js 2013-07-11 23:21:42 +0000
+++ test/test_model.js 2013-07-12 19:16:17 +0000
@@ -808,7 +808,10 @@
      var mysql = db.services.add({id: 'mysql', charm: 'precise/mysql-1'});
      var wordpress = db.services.add({
        id: 'wordpress',
- charm: 'precise/wordpress-1'});
+ charm: 'precise/wordpress-1',
+ config: {debug: 'no', username: 'admin'},
+ annotations: {'gui-x': 100, 'gui-y': 200, 'ignored': true}
+ });
      var rel0 = db.relations.add({
        id: 'relation-0',
        endpoints: [
@@ -820,13 +823,40 @@
      db.environment.set('defaultSeries', 'precise');

      // Add the charms so we can resolve them in the export.
- db.charms.add([{id: 'precise/mysql-1'}, {id: 'precise/wordpress-1'}]);
+ db.charms.add([{id: 'precise/mysql-1'},
+ {id: 'precise/wordpress-1',
+ config: {
+ options: {
+ debug: {
+ 'default': 'no'
+ },
+ username: {
+ 'default': 'root'
+ }
+ }
+ }
+ }
+ ]);
      var result = db.exportDeployer().envExport;
      var relation = result.relations[0];

      assert.equal(result.series, 'precise');
- assert.equal(result.services[0].charm, 'mysql');
- assert.equal(result.services[1].charm, 'wordpress');
+ assert.equal(result.services.mysql.charm, 'precise/mysql-1');
+ assert.equal(result.services.wordpress.charm, 'precise/wordpress-1');
+
+ // A default config value is skipped
+ assert.equal(result.services.wordpress.options.debug, undefined);
+ // A value changed from the default is exported
+ assert.equal(result.services.wordpress.options.username, 'admin');
+ // Ensure that mysql has no options object in the export as no
+ // non-default options are defined
+ assert.equal(result.services.mysql.options, undefined);
+
+ // Export position annotations.
+ assert.equal(result.services.wordpress.annotations['gui-x'], 100);
+ assert.equal(result.services.wordpress.annotations['gui-y'], 200);
+ // Note that ignored wasn't exported.
+ assert.equal(result.services.wordpress.annotations.ignored, undefined);

      assert.equal(relation[0], 'mysql:db');
      assert.equal(relation[1], 'w...

LGTM with a suggestion to go XXX vs "doesn't make sense" note.

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

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode919
app/models/models.js:919: // than relative which would make more sense
in an export.
should this be an XXX vs a note then if it doesn't make much sense as
is?

https://codereview.appspot.com/11227043/

LGTM with trivials, thank you!

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

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode889
app/models/models.js:889: // Process the serivce_options removing any
values
trivial: service_options

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode900
app/models/models.js:900: // Using package name here so the default
series
This comment is no longer correct. Delete?

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode919
app/models/models.js:919: // than relative which would make more sense
in an export.
Good point. File a bug? Low priority until someone asks for it.

https://codereview.appspot.com/11227043/

Thanks for the reviews

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

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode889
app/models/models.js:889: // Process the serivce_options removing any
values
On 2013/07/12 21:32:11, gary.poster wrote:
> trivial: service_options

Done.

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode900
app/models/models.js:900: // Using package name here so the default
series
On 2013/07/12 21:32:11, gary.poster wrote:
> This comment is no longer correct. Delete?

Ha, the "this is likely..." part was still valid, removed though as its
happened.

https://codereview.appspot.com/11227043/diff/1/app/models/models.js#newcode919
app/models/models.js:919: // than relative which would make more sense
in an export.
On 2013/07/12 21:15:00, rharding wrote:
> should this be an XXX vs a note then if it doesn't make much sense as
is?

XXX'd :)

https://codereview.appspot.com/11227043/

*** Submitted:

Many deployer export format fixes

Export Annotations
Skip export of default service options.
Services is a dict, not an array
Use qualified charm names

R=rharding, gary.poster, benjamin.saller
CC=
https://codereview.appspot.com/11227043

https://codereview.appspot.com/11227043/