Merge lp:~gary/juju-gui/fixSandboxDeployRace into lp:juju-gui/experimental

[Gary Poster]

Remove a race condition from sandbox deployment

Deploying to the sandbox will sometimes mis-position services, especially when you have a large bundle. I believe the cause is the race condition that this branch fixes.

I also simplified some code that I thought was confusing.

To QA, try deploying some big bundles. At the very least, it shouldn't be worse than it was before. If we're lucky, you'll see that the position is always right for the deployed bundles in the sandbox.

https://codereview.appspot.com/23800044/

[Gary Poster (gary)]

Reviewers: mp+194581_code.launchpad.net,

Message:
Please take a look.

Description:
Remove a race condition from sandbox deployment

Deploying to the sandbox will sometimes mis-position services,
especially when you have a large bundle. I believe the cause is the race
condition that this branch fixes.

I also simplified some code that I thought was confusing.

To QA, try deploying some big bundles. At the very least, it shouldn't
be worse than it was before. If we're lucky, you'll see that the
position is always right for the deployed bundles in the sandbox.

Contact me if you'd like some big bundles.

https://code.launchpad.net/~gary/juju-gui/fixSandboxDeployRace/+merge/194581

(do not edit description out of merge proposal)

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

Affected files (+40, -29 lines):
   [revision details]
   app/store/env/fakebackend.js
   test/test_fakebackend.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-11-01 23:04:50 +0000
+++ test/test_fakebackend.js 2013-11-08 18:57:35 +0000
@@ -337,6 +337,23 @@
            ' auto_id: %n\n' +
            ' ^');
      });
+
+ it('honors annotations', function() {
+ var options = {
+ annotations: {
+ 'gui-x': '-3',
+ 'gui-y': '5'
+ }
+ };
+ fakebackend.deploy('cs:precise/wordpress-15', callback, options);
+ var service = fakebackend.db.services.getById('wordpress');
+ assert.deepEqual(
+ options.annotations,
+ service.get('annotations')
+ );
+ var changes = fakebackend.nextAnnotations();
+ assert.deepEqual(changes.services, {wordpress: service});
+ });
    });

    describe('FakeBackend.setCharm', function() {

Index: app/store/env/fakebackend.js
=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js 2013-11-01 23:04:50 +0000
+++ app/store/env/fakebackend.js 2013-11-08 18:57:35 +0000
@@ -409,6 +409,8 @@
          constraints = options.constraints;
        }

+ var annotations = options.annotations || {};
+
        // In order for the constraints to support the python back end this
        // needs to be an array, so we are converting it back to an object
        // here so that the GUI displays it properly.
@@ -425,6 +427,17 @@
          constraintsMap = constraints;
        }

+ // We need to set charm default values for the options that have not
+ // been explicitly provided.
+ var charmOptions = charm.get('options') || {};
+ // "config" will hold the service's config values--it will be the
+ // result of this processing.
+ var config = {};
+ var explicitConfig = options.config || {};
+ Object.keys(charmOptions).forEach(function(key) {
+ config[key] = explicitConfig[key] || charmOptions[k...

[Brad Crittenden (bac)]

LGTM + QA-OK

https://codereview.appspot.com/23800044/

[Gary Poster (gary)]

*** Submitted:

Remove a race condition from sandbox deployment

Deploying to the sandbox will sometimes mis-position services,
especially when you have a large bundle. I believe the cause is the race
condition that this branch fixes.

I also simplified some code that I thought was confusing.

To QA, try deploying some big bundles. At the very least, it shouldn't
be worse than it was before. If we're lucky, you'll see that the
position is always right for the deployed bundles in the sandbox.

R=bac
CC=
https://codereview.appspot.com/23800044

https://codereview.appspot.com/23800044/

=== modified file 'app/store/env/fakebackend.js'
--- app/store/env/fakebackend.js	2013-11-01 23:04:50 +0000
+++ app/store/env/fakebackend.js	2013-11-08 19:58:38 +0000
@@ -409,6 +409,8 @@
         constraints = options.constraints;
       }
 
+      var annotations = options.annotations || {};
+
       // In order for the constraints to support the python back end this
       // needs to be an array, so we are converting it back to an object
       // here so that the GUI displays it properly.
@@ -425,6 +427,17 @@
         constraintsMap = constraints;
       }
 
+      // We need to set charm default values for the options that have not
+      // been explicitly provided.
+      var charmOptions = charm.get('options') || {};
+      // "config" will hold the service's config values--it will be the
+      // result of this processing.
+      var config = {};
+      var explicitConfig = options.config || {};
+      Object.keys(charmOptions).forEach(function(key) {
+        config[key] = explicitConfig[key] || charmOptions[key]['default'];
+      });
+
       var service = this.db.services.add({
         id: options.name,
         name: options.name,
@@ -432,22 +445,13 @@
         constraints: constraintsMap,
         exposed: false,
         subordinate: charm.get('is_subordinate'),
-        // Because we only send the user changed options now
-        // we need to mix those values in to the charm config
-        // options when creating a new model.
-        config: (function() {
-          var charmOptions = charm.get('options') || {};
-          var config = {};
-          if (!options.config) { options.config = {}; }
-          Object.keys(charmOptions).forEach(function(key) {
-            config[key] =
-                options.config[key] ||
-                (charmOptions[key] ? charmOptions[key]['default'] : undefined);
-          });
-          return config;
-        })()
+        annotations: annotations,
+        config: config
       });
       this.changes.services[service.get('id')] = [service, true];
+      if (Object.keys(annotations).length) {
+        this.annotations.services[service.get('id')] = service;
+      }
 
       var unitCount = options.unitCount;
       if (!Y.Lang.isValue(unitCount) && !charm.get('is_subordinate')) {
@@ -1522,28 +1526,16 @@
 
       Y.batch.apply(this, servicePromises)
       .then(function(serviceDeployResult) {
+            // Expose, if requested.
             serviceDeployResult.forEach(function(sdr) {
-              // Update export elements that 'deploy'
-              // doesn't handle
-              var service = sdr.service;
-              var serviceId = service.get('id');
+              var serviceId = sdr.service.get('id');
               var serviceData = ingestedData.services[serviceId];
-
-              // Force the annotation update (deploy doesn't handle this).
-              var anno = serviceData.annotations;
-              if (anno) {
-                self.updateAnnotations(service.get('id'), anno);
-              }
-
-              // Expose
               if (serviceData.expose) {
                 self.expose(serviceId);
               }
-
-              self.changes.services[sdr.service.get('id')] = [
-                sdr.service, true];
             });
 
+            // Create requested relations.
             ingestedData.relations.forEach(function(relationData) {
               var relResult = self.addRelation(
                   relationData[0], relationData[1], true);

=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js	2013-11-01 23:04:50 +0000
+++ test/test_fakebackend.js	2013-11-08 19:58:38 +0000
@@ -337,6 +337,23 @@
           '    auto_id: %n\n' +
           '             ^');
     });
+
+    it('honors annotations', function() {
+      var options = {
+        annotations: {
+          'gui-x': '-3',
+          'gui-y': '5'
+        }
+      };
+      fakebackend.deploy('cs:precise/wordpress-15', callback, options);
+      var service = fakebackend.db.services.getById('wordpress');
+      assert.deepEqual(
+          options.annotations,
+          service.get('annotations')
+      );
+      var changes = fakebackend.nextAnnotations();
+      assert.deepEqual(changes.services, {wordpress: service});
+    });
   });
 
   describe('FakeBackend.setCharm', function() {