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

[Benjamin Saller]

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://codereview.appspot.com/11227043/

[Benjamin Saller (bcsaller)]

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...

[Richard Harding (rharding)]

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/

[Gary Poster (gary)]

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/

[Benjamin Saller (bcsaller)]

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/

[Benjamin Saller (bcsaller)]

*** 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/

=== modified file 'app/models/models.js'
--- app/models/models.js	2013-07-08 21:52:00 +0000
+++ app/models/models.js	2013-07-12 19:20:34 +0000
@@ -871,7 +871,7 @@
           result = {
             envExport: {
               series: this.environment.get('defaultSeries'),
-              services: [],
+              services: {},
               relations: []
             }
           };
@@ -879,22 +879,50 @@
       serviceList.each(function(service) {
         var units = service.units;
         var charm = self.charms.getById(service.get('charm'));
-        if (service.get('pending') === true) {return;}
+        var serviceOptions = {};
+        var charmOptions = charm.get('config.options');
+
+        if (service.get('pending') === true) {
+          return;
+        }
+
+        // Process the serivce_options removing any values
+        // that are the default value for the charm.
+        Y.each(service.get('config'), function(value, key) {
+          var optionData = charmOptions && charmOptions[key];
+          if (!optionData || (optionData && optionData['default'] &&
+              (value !== optionData['default']))) {
+            serviceOptions[key] = value;
+          }
+        });
+
         var serviceData = {
           // Using package name here so the default series
           // is picked up. This will likely have to be the full
           // path in the future.
-          charm: charm.get('package_name'),
-          options: service.get('config'),
+          charm: charm.get('id'),
           // Test models or ghosts might not have a units LazyModelList.
           num_units: units && units.size() || 1
         };
+        if (serviceOptions && Y.Object.size(serviceOptions) >= 1) {
+          serviceData.options = serviceOptions;
+        }
         // Add constraints
         var constraints = service.get('constraintsStr');
         if (constraints) {
           serviceData.constraints = constraints;
         }
-        result.envExport.services.push(serviceData);
+
+        var annotations = service.get('annotations');
+        if (annotations && annotations['gui-x']) {
+          // Only expose position. Currently these are position absolute rather
+          // than relative which would make more sense in an export.
+          serviceData.annotations = {
+            'gui-x': annotations['gui-x'],
+            'gui-y': annotations['gui-y']
+          };
+        }
+        result.envExport.services[service.get('id')] = serviceData;
       });
 
       relationList.each(function(relation) {
 
=== 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:20:34 +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], 'wordpress:app');