Merge into trunk : exportBugs : Code : juju-gui

Status:

Merged

Merged at revision:

1149

Proposed branch:

lp://staging/~gary/juju-gui/exportBugs

Merge into:

lp://staging/juju-gui/experimental

Diff against target:

182 lines (+74/-12)

5 files modified

app/models/models.js (+7/-3)
app/store/env/fakebackend.js (+7/-3)
test/data/wp-deployer.yaml (+2/-5)
test/test_fakebackend.js (+12/-1)
test/test_model.js (+46/-0)

To merge this branch:

bzr merge lp://staging/~gary/juju-gui/exportBugs

Related bugs:

Bug #1241782: juju-gui export does not include exposed ports	High	Fix Released
Bug #1243407: Export does not include many changed values	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-10-23	Pending
Review via email: mp+192286@code.staging.launchpad.net

Description of the change

Fix export and import bugs

Our support for exporting and importing config values and expose flags was buggy. This fixes four discrete but related bugs.

- Exporting config values would exclude those that had false-y default values.
- We did not export the expose flag.
- Importing config values did not work.
- We called the expose flag "exposed", which is not what the actual delpoyer expects.

To qa, open up the sandbox, deploy apache with "enable_modules" set to something or other, and expose it. Then export the environment. When you look at the service in the file, you should see "expose: true" and "enable_modules: mod_proxy" or whatever value you set. Then reload the GUI and drag the file into the sandbox. When the service has loaded, you should see that it is exposed, and when you look at the config in the inspector, you should see your value in the "enable_modules" field.

https://codereview.appspot.com/15400052/

Revision history for this message

Gary Poster (gary) wrote on 2013-10-23:

#

Download full text (8.0 KiB)

Reviewers: mp+192286_code.launchpad.net,

Message:
Please take a look.

Description:
Fix export and import bugs

Our support for exporting and importing config values and expose flags
was buggy. This fixes four discrete but related bugs.

  - Exporting config values would exclude those that had false-y default
values.
  - We did not export the expose flag.
  - Importing config values did not work.
  - We called the expose flag "exposed", which is not what the actual
delpoyer expects.

To qa, open up the sandbox, deploy apache with "enable_modules" set to
something or other, and expose it. Then export the environment. When
you look at the service in the file, you should see "expose: true" and
"enable_modules: mod_proxy" or whatever value you set. Then reload the
GUI and drag the file into the sandbox. When the service has loaded,
you should see that it is exposed, and when you look at the config in
the inspector, you should see your value in the "enable_modules" field.

https://code.launchpad.net/~gary/juju-gui/exportBugs/+merge/192286

(do not edit description out of merge proposal)

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

Affected files (+76, -12 lines):
   A [revision details]
   M app/models/models.js
   M app/store/env/fakebackend.js
   M test/data/wp-deployer.yaml
   M test/test_fakebackend.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_fakebackend.js
=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js 2013-10-15 08:16:18 +0000
+++ test/test_fakebackend.js 2013-10-23 05:23:16 +0000
@@ -528,6 +528,10 @@
});
});

+ beforeEach(function() {
+ fakebackend = utils.makeFakeBackend();
+ });
+
      afterEach(function() {
        if (fakebackend) {
          fakebackend.destroy();
@@ -555,9 +559,16 @@

              // Verify config.
              var wordpress = fakebackend.db.services.getById('wordpress');
+ var wordpressCharm = fakebackend.db.charms.getById(
+ 'cs:precise/wordpress-15');
              var mysql = fakebackend.db.services.getById('mysql');
- assert.equal(wordpress.get('config.engine'), 'nginx');
+ // This value is different from the default (nginx).
+
assert.equal(wordpressCharm.get('options.engine.default'), 'nginx');
+ assert.equal(wordpress.get('config.engine'), 'apache');
+ // This value is the default, as provided by the charm.
              assert.equal(wordpress.get('config.tuning'), 'single');
+ assert.isTrue(wordpress.get('exposed'));
+ assert.isFalse(mysql.get('exposed'));

// Constraints
var constraints = mysql.get('constraints');

Index: test/test_model.js
=== modified file 'test/test_model.js'
--- test/test_model.js 2013-10-16 11:55:56 +0000
+++ test/test_model.js 2013-10-23 03:34:59 +0000
@@...

Reviewers: mp+192286_code.launchpad.net,

Message:
Please take a look.

Description:
Fix export and import bugs

Our support for exporting and importing config values and expose flags
was buggy.  This fixes four discrete but related bugs.

- Exporting config values would exclude those that had false-y default
values.
  - We did not export the expose flag.
  - Importing config values did not work.
  - We called the expose flag "exposed", which is not what the actual
delpoyer expects.

To qa, open up the sandbox, deploy apache with "enable_modules" set to
something or other, and expose it.  Then export the environment.  When
you look at the service in the file, you should see "expose: true" and
"enable_modules: mod_proxy" or whatever value you set.  Then reload the
GUI and drag the file into the sandbox.  When the service has loaded,
you should see that it is exposed, and when you look at the config in
the inspector, you should see your value in the "enable_modules" field.

https://code.launchpad.net/~gary/juju-gui/exportBugs/+merge/192286

(do not edit description out of merge proposal)

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

Affected files (+76, -12 lines):
   A [revision details]
   M app/models/models.js
   M app/store/env/fakebackend.js
   M test/data/wp-deployer.yaml
   M test/test_fakebackend.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: rick.harding@canonical.com-20131022004208-jvdgzzn0nvrlvo67
+New revision: gary.poster@canonical.com-20131023052316-spcuxto9s4rjbc5m

Index: test/test_fakebackend.js
=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js	2013-10-15 08:16:18 +0000
+++ test/test_fakebackend.js	2013-10-23 05:23:16 +0000
@@ -528,6 +528,10 @@
        });
      });

+    beforeEach(function() {
+      fakebackend = utils.makeFakeBackend();
+    });
+
      afterEach(function() {
        if (fakebackend) {
          fakebackend.destroy();
@@ -555,9 +559,16 @@

// Verify config.
              var wordpress = fakebackend.db.services.getById('wordpress');
+            var wordpressCharm = fakebackend.db.charms.getById(
+                'cs:precise/wordpress-15');
              var mysql = fakebackend.db.services.getById('mysql');
-            assert.equal(wordpress.get('config.engine'), 'nginx');
+            // This value is different from the default (nginx).
+             
assert.equal(wordpressCharm.get('options.engine.default'), 'nginx');
+            assert.equal(wordpress.get('config.engine'), 'apache');
+            // This value is the default, as provided by the charm.
              assert.equal(wordpress.get('config.tuning'), 'single');
+            assert.isTrue(wordpress.get('exposed'));
+            assert.isFalse(mysql.get('exposed'));

// Constraints
              var constraints = mysql.get('constraints');

Index: test/test_model.js
=== modified file 'test/test_model.js'
--- test/test_model.js	2013-10-16 11:55:56 +0000
+++ test/test_model.js	2013-10-23 03:34:59 +0000
@@ -867,6 +867,52 @@
        assert.isUndefined(result.services.puppet.num_units);
      });

+    it('exports non-default options', function() {
+      db.services.add({
+        id: 'wordpress',
+        charm: 'precise/wordpress-1',
+        config: {one: '1', two: '2', three: '3', four: '4', five: true},
+        annotations: {'gui-x': 100, 'gui-y': 200}
+      });
+      db.charms.add([{id: 'precise/mysql-1'},
+            {id: 'precise/wordpress-1',
+              options: {
+                one: {
+                  'default': ''
+                },
+                two: {
+                  'default': null
+                },
+                three: {
+                  'default': undefined
+                },
+                four: {
+                  'default': '0'
+                },
+                five: {
+                  'default': false
+                }
+              }
+            }
+          ]);
+      var result = db.exportDeployer().envExport;
+      assert.equal(result.services.wordpress.options.one, '1');
+      assert.equal(result.services.wordpress.options.two, '2');
+      assert.equal(result.services.wordpress.options.three, '3');
+      assert.equal(result.services.wordpress.options.four, '4');
+      assert.equal(result.services.wordpress.options.five, true);
+    });
+
+    it('exports exposed flag', function() {
+      db.services.add({id: 'wordpress', charm: 'precise/wordpress-4'});
+      db.charms.add([{id: 'precise/wordpress-4'}]);
+      var result = db.exportDeployer().envExport;
+      assert.isUndefined(result.services.wordpress.expose);
+      db.services.getById('wordpress').set('exposed', true);
+      result = db.exportDeployer().envExport;
+      assert.isTrue(result.services.wordpress.expose);
+    });
+
    });

describe('service models', function() {

Index: app/models/models.js
=== modified file 'app/models/models.js'
--- app/models/models.js	2013-10-16 11:55:56 +0000
+++ app/models/models.js	2013-10-23 03:34:59 +0000
@@ -1168,9 +1168,9 @@
          // that are the default value for the charm.
          Y.each(service.get('config'), function(value, key) {
            var optionData = charmOptions && charmOptions[key];
-          if ((!optionData && value !== undefined) ||
-              (optionData && optionData['default'] &&
-              (value !== optionData['default']))) {
+          var defaultVal = optionData && optionData['default'];
+          var hasDefault = Y.Lang.isValue(defaultVal);
+          if (Y.Lang.isValue(value) && (!hasDefault || value !==  
defaultVal)) {
              serviceOptions[key] = value;
            }
          });
@@ -1190,6 +1190,10 @@
            serviceData.constraints = constraints;
          }

+        if (service.get('exposed')) {
+          serviceData.expose = true;
+        }
+
          // XXX: Only expose position. Currently these are position absolute
          // rather than relative.
          var anno = service.get('annotations');

Index: test/data/wp-deployer.yaml
=== modified file 'test/data/wp-deployer.yaml'
--- test/data/wp-deployer.yaml	2013-10-10 17:00:09 +0000
+++ test/data/wp-deployer.yaml	2013-10-23 05:17:56 +0000
@@ -9,8 +9,6 @@
          "block-size": "5"
          "dataset-size": "80%"
          flavor: distro
-        'gui-x': 50
-        'gui-y': 50
          "ha-bindiface": eth0
          "ha-mcastport": "5411"
          "max-connections": "-1"
@@ -31,13 +29,12 @@
        num_units: 2
        options:
          debug: "no"
-        engine: nginx
-        tuning: single
+        engine: apache
          "wp-content": ""
        annotations:
          "gui-x": 510
          "gui-y": 184
-      exposed: true
+      expose: true
    relations:
      - - "wordpress:db"
        - "mysql:db"

Index: app/store/env/fakebackend.js
=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js	2013-10-15 19:33:04 +0000
+++ app/store/env/fakebackend.js	2013-10-23 05:17:56 +0000
@@ -1368,9 +1368,9 @@
        var seen = [];

/**
-        Helper to build out an inheritence chain
+        Helper to build out an inheritance chain

-        @method setupinheritance
+        @method setupInheritance
          @param {Object} base object currently being inspected.
          @param {Array} baseList chain of ancestors to later inherit.
          @param {Object} bundleData import data used to resolve ancestors.
@@ -1497,6 +1497,10 @@
          if (!serviceData.unitCount) {
            serviceData.unitCount = serviceData.num_units || 1;
          }
+        if (serviceData.options) {
+          serviceData.config = serviceData.options;
+          delete serviceData.options;
+        }
          servicePromises.push(
              self.promiseDeploy(serviceData.charm, serviceData));
        });
@@ -1530,7 +1534,7 @@
                }

// Expose
-              if (serviceData.exposed) {
+              if (serviceData.expose) {
                  self.expose(serviceId);
                }

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-10-23:

#

Thanks for this fix!
LGTM QA OK!

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

https://codereview.appspot.com/15400052/diff/1/app/models/models.js#newcode1193
app/models/models.js:1193: if (service.get('exposed')) {
Maybe we should change what the deployer expects or our service
attribute to match instead of doing this conversion.

https://codereview.appspot.com/15400052/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/15400052/diff/1/test/test_fakebackend.js#newcode532
test/test_fakebackend.js:532: fakebackend = utils.makeFakeBackend();
How did this work before?

https://codereview.appspot.com/15400052/

Revision history for this message

Gary Poster (gary) wrote on 2013-10-23:

#

*** Submitted:

Fix export and import bugs

Our support for exporting and importing config values and expose flags
was buggy. This fixes four discrete but related bugs.

  - Exporting config values would exclude those that had false-y default
values.
  - We did not export the expose flag.
  - Importing config values did not work.
  - We called the expose flag "exposed", which is not what the actual
delpoyer expects.

To qa, open up the sandbox, deploy apache with "enable_modules" set to
something or other, and expose it. Then export the environment. When
you look at the service in the file, you should see "expose: true" and
"enable_modules: mod_proxy" or whatever value you set. Then reload the
GUI and drag the file into the sandbox. When the service has loaded,
you should see that it is exposed, and when you look at the config in
the inspector, you should see your value in the "enable_modules" field.

R=jeff.pihach
CC=
https://codereview.appspot.com/15400052

https://codereview.appspot.com/15400052/

juju-gui

Merge lp://staging/~gary/juju-gui/exportBugs into lp://staging/juju-gui/experimental

Commit message

Description of the change

Preview Diff

Subscribers