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

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], 'wordpress:app');

Index: app/models/models.js
=== 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:15:09 +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) {