Merge lp:~frankban/juju-gui/bug-1169858-life into lp:juju-gui/experimental

[Francesco Banconi]

Handle service life

The Life change included in the juju-core delta stream is
now stored in the service model, and used to avoid displaying
dying services in the topology view.

QA:

- bootstrap a juju-core env;
- deploy a service;
- connect the GUI to the env;
- use the GUI to destroy the service;
- suddenly refresh (or visit the GUI from another browser tab).

At this point, even if the service could be still
present in the db, it should not be alive and you should
not see it in the topology view.

https://codereview.appspot.com/8824044/

[Francesco Banconi (frankban)]

Reviewers: mp+159356_code.launchpad.net,

Message:
Please take a look.

Description:
Handle service life

The Life change included in the juju-core delta stream is
now stored in the service model, and used to avoid displaying
dying services in the topology view.

QA:

- bootstrap a juju-core env;
- deploy a service;
- connect the GUI to the env;
- use the GUI to destroy the service;
- suddenly refresh (or visit the GUI from another browser tab).

At this point, even if the service could be still
present in the db, it should not be alive and you should
not see it in the topology view.

https://code.launchpad.net/~frankban/juju-gui/bug-1169858-life/+merge/159356

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/models/handlers.js
   M app/models/models.js
   M app/views/topology/service.js
   M test/test_fakebackend.js
   M test/test_model.js
   M test/test_model_handlers.js
   M test/test_service_module.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: app/models/handlers.js
=== modified file 'app/models/handlers.js'
--- app/models/handlers.js 2013-04-16 12:24:34 +0000
+++ app/models/handlers.js 2013-04-17 10:50:07 +0000
@@ -166,6 +166,7 @@
          id: change.Name,
          charm: change.CharmURL,
          exposed: change.Exposed,
+ life: change.Life,
          constraints: change.Constraints || {}
        };
        db.services.process_delta(action, data);

Index: app/models/models.js
=== modified file 'app/models/models.js'
--- app/models/models.js 2013-04-12 16:04:02 +0000
+++ app/models/models.js 2013-04-17 11:23:44 +0000
@@ -91,7 +91,21 @@
    });
    models.Environment = Environment;

- var Service = Y.Base.create('service', Y.Model, [], {}, {
+ var ALIVE = 'alive';
+
+ var Service = Y.Base.create('service', Y.Model, [], {
+
+ /**
+ Return true if this service life is "alive", false otherwise.
+
+ @method isAlive
+ @return {Boolean} Whether this service is alive.
+ */
+ isAlive: function() {
+ return this.get('life') === ALIVE;
+ }
+
+ }, {
      ATTRS: {
        displayName: {
          /**
@@ -115,6 +129,9 @@
        pending: {
          value: false
        },
+ life: {
+ value: ALIVE
+ },
        unit_count: {},
        aggregated_status: {}
      }
@@ -124,6 +141,18 @@
    var ServiceList = Y.Base.create('serviceList', Y.ModelList, [], {
      model: Service,

+ /**
+ Return a list of alive model instances.
+
+ @method alive
+ @return {Y.ModelList} The model instances having life === 'alive'.
+ */
+ alive: function() {
+ return this.filter({asList: true}, function(model) {
+ return model.isAlive();
+ });
+ },
+
      process_delta: function(action, data) {
        _process_delta(this, action, data, {exposed: false});
      }

...

[Gary Poster (gary)]

Looks good. QAing.

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

https://codereview.appspot.com/8824044/diff/1/app/models/models.js#newcode99
app/models/models.js:99: Return true if this service life is "alive",
false otherwise.
On 2013/04/17 12:37:59, benji wrote:
> It might be nice to explain (ether here or in "alive" below) what it
means for a
> service to be "alive".
+1

https://codereview.appspot.com/8824044/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8824044/diff/1/test/test_model.js#newcode665
test/test_model.js:665: it('instances identify if they are alive',
function() {
(and also verifies that the default state is alive :-) )

https://codereview.appspot.com/8824044/

[Gary Poster (gary)]

LGTM. QA went well. Thank you!

Gary

https://codereview.appspot.com/8824044/

[lp:~frankban/juju-gui/bug-1169858-life]

570. By Francesco Banconi on 2013-04-17

Changes as per review.

571. By Francesco Banconi on 2013-04-17

Merged trunk.

[Francesco Banconi (frankban)]

*** Submitted:

Handle service life

The Life change included in the juju-core delta stream is
now stored in the service model, and used to avoid displaying
dying services in the topology view.

QA:

- bootstrap a juju-core env;
- deploy a service;
- connect the GUI to the env;
- use the GUI to destroy the service;
- suddenly refresh (or visit the GUI from another browser tab).

At this point, even if the service could be still
present in the db, it should not be alive and you should
not see it in the topology view.

R=benji, gary.poster
CC=
https://codereview.appspot.com/8824044

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

https://codereview.appspot.com/8824044/diff/1/app/models/models.js#newcode99
app/models/models.js:99: Return true if this service life is "alive",
false otherwise.
On 2013/04/17 12:37:59, benji wrote:
> It might be nice to explain (ether here or in "alive" below) what it
means for a
> service to be "alive".

Done.

https://codereview.appspot.com/8824044/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/8824044/diff/1/test/test_model.js#newcode665
test/test_model.js:665: it('instances identify if they are alive',
function() {
On 2013/04/17 12:54:26, gary.poster wrote:
> (and also verifies that the default state is alive :-) )

Indeed! Added a comment.

https://codereview.appspot.com/8824044/

[Francesco Banconi (frankban)]

Thank you both for the reviews!

https://codereview.appspot.com/8824044/

=== modified file 'app/models/handlers.js'
--- app/models/handlers.js	2013-04-16 12:24:34 +0000
+++ app/models/handlers.js	2013-04-17 13:37:24 +0000
@@ -166,6 +166,7 @@
         id: change.Name,
         charm: change.CharmURL,
         exposed: change.Exposed,
+        life: change.Life,
         constraints: change.Constraints || {}
       };
       db.services.process_delta(action, data);

=== modified file 'app/models/models.js'
--- app/models/models.js	2013-04-12 16:04:02 +0000
+++ app/models/models.js	2013-04-17 13:37:24 +0000
@@ -91,7 +91,26 @@
   });
   models.Environment = Environment;
 
-  var Service = Y.Base.create('service', Y.Model, [], {}, {
+  var ALIVE = 'alive';
+
+  var Service = Y.Base.create('service', Y.Model, [], {
+
+    /**
+      Return true if this service life is "alive", false otherwise.
+
+      A model instance is alive if its life cycle (i.e. the "life" attribute)
+      is set to "alive". Other possible values, as they arrive from the
+      juju-core delta stream, are "dying" and "dead", in which cases the
+      service is not considered alive.
+
+      @method isAlive
+      @return {Boolean} Whether this service is alive.
+     */
+    isAlive: function() {
+      return this.get('life') === ALIVE;
+    }
+
+  }, {
     ATTRS: {
       displayName: {
         /**
@@ -115,6 +134,9 @@
       pending: {
         value: false
       },
+      life: {
+        value: ALIVE
+      },
       unit_count: {},
       aggregated_status: {}
     }
@@ -124,6 +146,18 @@
   var ServiceList = Y.Base.create('serviceList', Y.ModelList, [], {
     model: Service,
 
+    /**
+      Return a list of alive model instances.
+
+      @method alive
+      @return {Y.ModelList} The model instances having life === 'alive'.
+    */
+    alive: function() {
+      return this.filter({asList: true}, function(model) {
+        return model.isAlive();
+      });
+    },
+
     process_delta: function(action, data) {
       _process_delta(this, action, data, {exposed: false});
     }

=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js	2013-04-17 01:04:26 +0000
+++ app/views/topology/service.js	2013-04-17 13:37:24 +0000
@@ -353,7 +353,7 @@
       var vis = topo.vis;
       var db = topo.get('db');
 
-      views.toBoundingBoxes(this, db.services, topo.service_boxes);
+      views.toBoundingBoxes(this, db.services.alive(), topo.service_boxes);
 
       // Nodes are mapped by modelId tuples.
       this.node = vis.selectAll('.service')

=== modified file 'test/test_fakebackend.js'
--- test/test_fakebackend.js	2013-04-16 20:21:27 +0000
+++ test/test_fakebackend.js	2013-04-17 13:37:24 +0000
@@ -102,6 +102,7 @@
         initialized: true,
         name: 'wordpress',
         pending: false,
+        life: 'alive',
         subordinate: false,
         unit_count: undefined
       });

=== modified file 'test/test_model.js'
--- test/test_model.js	2013-04-12 22:57:49 +0000
+++ test/test_model.js	2013-04-17 13:37:24 +0000
@@ -643,3 +643,39 @@
     instance.get('recent_commit_count').should.equal(3);
   });
 });
+
+
+describe('service models', function() {
+  var models, list, django, rails, mysql;
+
+  before(function(done) {
+    YUI(GlobalConfig).use(['juju-models'], function(Y) {
+      models = Y.namespace('juju.models');
+      done();
+    });
+  });
+
+  beforeEach(function() {
+    django = new models.Service({id: 'django'});
+    rails = new models.Service({id: 'rails', life: 'dying'});
+    mysql = new models.Service({id: 'mysql', life: 'dead'});
+    list = new models.ServiceList({items: [rails, django, mysql]});
+  });
+
+  it('instances identify if they are alive', function() {
+    // This test also verifies that the default state is "alive".
+    assert.isTrue(django.isAlive());
+  });
+
+  it('instances identify if they are not alive (dying or dead)', function() {
+    assert.isFalse(rails.isAlive());
+    assert.isFalse(mysql.isAlive());
+  });
+
+  it('can be filtered so that it returns only alive models', function() {
+    var filtered = list.alive();
+    assert.strictEqual(1, filtered.size());
+    assert.deepEqual([django], filtered.toArray());
+  });
+
+});

=== modified file 'test/test_model_handlers.js'
--- test/test_model_handlers.js	2013-04-15 19:00:44 +0000
+++ test/test_model_handlers.js	2013-04-17 13:37:24 +0000
@@ -193,7 +193,8 @@
           Name: 'django',
           CharmURL: 'cs:precise/django-42',
           Exposed: true,
-          Constraints: constraints
+          Constraints: constraints,
+          Life: 'alive'
         };
         serviceInfo(db, 'add', change);
         assert.strictEqual(1, db.services.size());
@@ -202,6 +203,7 @@
         assert.strictEqual('cs:precise/django-42', service.get('charm'));
         assert.isTrue(service.get('exposed'));
         assert.deepEqual(constraints, service.get('constraints'));
+        assert.strictEqual('alive', service.get('life'));
       });
 
       it('updates a service in the database', function() {
@@ -213,7 +215,8 @@
         var change = {
           Name: 'wordpress',
           CharmURL: 'cs:quantal/wordpress-11',
-          Exposed: false
+          Exposed: false,
+          Life: 'dying'
         };
         serviceInfo(db, 'change', change);
         assert.strictEqual(1, db.services.size());
@@ -221,6 +224,7 @@
         var service = db.services.getById('wordpress');
         assert.strictEqual('cs:quantal/wordpress-11', service.get('charm'));
         assert.isFalse(service.get('exposed'));
+        assert.strictEqual('dying', service.get('life'));
       });
 
       it('if constraints are not in the change stream they are {}',

=== modified file 'test/test_service_module.js'
--- test/test_service_module.js	2013-04-08 21:40:05 +0000
+++ test/test_service_module.js	2013-04-17 13:37:24 +0000
@@ -311,4 +311,23 @@
     // The flag is reset when encountered and ignored.
     assert.isFalse(topo.ignoreServiceClick);
   });
+
+  it('should hide dying or dead services', function() {
+    var haproxy = db.services.getById('haproxy'); // Added in beforeEach.
+    db.services.add([
+      {id: 'rails', life: 'dying'},
+      {id: 'mysql', life: 'dead'}
+    ]);
+    var django = db.services.add({id: 'django'});
+    serviceModule.update();
+    var boxes = topo.service_boxes;
+    // There are four services in total.
+    assert.strictEqual(4, db.services.size());
+    // But only two of those are actually displayed.
+    assert.strictEqual(2, Y.Object.size(boxes));
+    // And they are the alive ones.
+    assert.deepPropertyVal(boxes, 'haproxy.model', haproxy);
+    assert.deepPropertyVal(boxes, 'django.model', django);
+  });
+
 });