juju-gui

Merge lp:~hatch/juju-gui/inspector-relation-status into lp:juju-gui/experimental

  1. inspector-relation-status
  2. Merge into trunk
Proposed by Jeff Pihach
Status: Merged
Merged at revision: 1212
Proposed branch: lp:~hatch/juju-gui/inspector-relation-status
Merge into: lp:juju-gui/experimental
Diff against target: 306 lines (+82/-88)
8 files modified
app/models/models.js (+31/-22)
app/templates/service-relations-list.handlebars (+1/-1)
app/templates/service-relations-viewlet.handlebars (+1/-1)
app/templates/service-relations.handlebars (+0/-40)
app/views/utils.js (+6/-0)
app/views/viewlets/service-relations.js (+3/-7)
lib/views/juju-inspector.less (+11/-0)
test/test_model.js (+29/-17)
To merge this branch: bzr merge lp:~hatch/juju-gui/inspector-relation-status
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+196770@code.launchpad.net

Description of the change

Adds relation status to inspector relation panel

The overall relation status is now indicated on the inspector relation
panel using the same icons as the unit status. If any unit has a relation
error the entire relation is shown as in error.

https://codereview.appspot.com/33130043/

To post a comment you must log in.
Revision history for this message
Jeff Pihach (hatch) wrote :

Reviewers: mp+196770_code.launchpad.net,

Message:
Please take a look.

Description:
Adds relation status to inspector relation panel

The overall relation status is now indicated on the inspector relation
panel using the same icons as the unit status. If any unit has a
relation
error the entire relation is shown as in error.

To QA:
Sandbox:
Deploy Mysql & Wordpress with 50 units each
Relate them
See the inspector relations panel, there should be a green icon
Wait for a relation error
See the inspector relations panel, there should be a red icon
Reduce the units on your inspector to 1
See the inspector relations panel, there should be a green icon
   because it's very likely the unit in error is gone

LXC
Deploy two instances of failtester
set ones relation fail setting to true
relate them
the inspector relations panel should show an error
resolve the error
the inspector relations panel should show green

https://code.launchpad.net/~hatch/juju-gui/inspector-relation-status/+merge/196770

(do not edit description out of merge proposal)

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

Affected files (+77, -82 lines):
   A [revision details]
   M app/models/models.js
   M app/templates/service-relations-list.handlebars
   M app/templates/service-relations-viewlet.handlebars
   D app/templates/service-relations.handlebars
   M app/views/utils.js
   M app/views/viewlets/service-relations.js
   M lib/views/juju-inspector.less
   M test/test_model.js

Revision history for this message
Jeff Pihach (hatch) wrote :

reviewer notes

https://codereview.appspot.com/33130043/diff/1/app/templates/service-relations.handlebars
File app/templates/service-relations.handlebars (left):

https://codereview.appspot.com/33130043/diff/1/app/templates/service-relations.handlebars#oldcode1
app/templates/service-relations.handlebars:1: {{> service-header}}
Template from old inspector

https://codereview.appspot.com/33130043/

Revision history for this message
Jeff Pihach (hatch) wrote :

Please take a look.

https://codereview.appspot.com/33130043/

lp:~hatch/juju-gui/inspector-relation-status updated
1214. By Jeff Pihach

fixed issue where the first error status delta would get ignored for relation errors

Revision history for this message
Jeff Pihach (hatch) wrote :

reviewer notes v2

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js#newcode605
app/models/models.js:605: if (state === 'error') {
Fix for bug from first rev (see test_model)

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js#newcode136
test/test_model.js:136: id: 'wordpress/2',
The order of these is important to make sure that the first unit error,
if it's a relation error gets caught properly (bug from first rev)

https://codereview.appspot.com/33130043/

Revision history for this message
Gary Poster (gary) wrote :

LGTM with trivials and sandbox QA good. waiting on QA on ec2.

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js#newcode591
app/models/models.js:591: @return {Array} An array of the aggregate map
and relation errors
Thank you for improving names and docs. Nice.

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js#newcode614
app/models/models.js:614: if (farService) {
this code fixes peer relation issues?

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js#newcode136
test/test_model.js:136: id: 'wordpress/2',
On 2013/11/26 20:42:45, jeff.pihach wrote:
> The order of these is important to make sure that the first unit
 error, if it's
> a relation error gets caught properly (bug from first rev)

In that case could you elaborate in a code comment what's going on? All
I see in the code is "one of these has a relation error and one has an
install error," not something subtle about ordering.

https://codereview.appspot.com/33130043/

Revision history for this message
Gary Poster (gary) wrote :

QA OK on real environment also. Thanks! These are really cool changes
you are making.

https://codereview.appspot.com/33130043/

Revision history for this message
Jeff Pihach (hatch) wrote :

Thanks for the review! Changes landing

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js#newcode136
test/test_model.js:136: id: 'wordpress/2',
good call, done

https://codereview.appspot.com/33130043/

Revision history for this message
Jeff Pihach (hatch) wrote :

*** Submitted:

Adds relation status to inspector relation panel

The overall relation status is now indicated on the inspector relation
panel using the same icons as the unit status. If any unit has a
relation
error the entire relation is shown as in error.

R=gary.poster
CC=
https://codereview.appspot.com/33130043

https://codereview.appspot.com/33130043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'app/models/models.js'
--- app/models/models.js 2013-11-21 23:21:49 +0000
+++ app/models/models.js 2013-11-26 20:42:30 +0000
@@ -186,7 +186,7 @@
186 this._events.push(186 this._events.push(
187 this.get('relations').on(187 this.get('relations').on(
188 ['*:change', '*:add', '*:remove'], function(e) {188 ['*:change', '*:add', '*:remove'], function(e) {
189 this.set('aggregateRelations', e);189 this.set('relationChangeTrigger', e);
190 }, this));190 }, this));
191 },191 },
192192
@@ -340,14 +340,16 @@
340 relations: {},340 relations: {},
341341
342 /**342 /**
343 An aggregate of the relations statuses that we use to trigger343 When there is a change to the relation or to any units in the relation
344 databinding changes344 this value is changed to trigger changes in the inspector.
345345
346 @attribute aggregateRelations346 @attribute relationChangeTrigger
347 @default {}347 @default {}
348 @type {Object}348 @type {Object}
349 */349 */
350 aggregateRelations: {},350 relationChangeTrigger: {
351 value: {}
352 },
351353
352 /**354 /**
353 An aggregate of the relation errors that we use to trigger355 An aggregate of the relation errors that we use to trigger
@@ -580,11 +582,14 @@
580 return result;582 return result;
581 },583 },
582584
583 /*585 /**
584 * Return information about the state of the set of units for a586 Returns information about the state of the set of units for a given
585 * given service in the form of a map of agent states:587 service in the form of a map of agent states.
586 * state => number of units in that state588
587 */589 @method get_informative_states_for_service
590 @param {Object} service The service model.
591 @return {Array} An array of the aggregate map and relation errors
592 */
588 get_informative_states_for_service: function(service) {593 get_informative_states_for_service: function(service) {
589 var aggregate_map = {},594 var aggregate_map = {},
590 relationError = {},595 relationError = {},
@@ -596,22 +601,24 @@
596 aggregate_map[state] = 1;601 aggregate_map[state] = 1;
597 } else {602 } else {
598 aggregate_map[state] += 1;603 aggregate_map[state] += 1;
599 if (state === 'error') {604 }
600 // If in error status then we need to parse out why it's in error.605 if (state === 'error') {
601 var info = unit.agent_state_info;606 // If in error status then we need to parse out why it's in error.
602 if (info !== undefined && info.indexOf('failed') > -1) {607 var info = unit.agent_state_info;
603 // If we parse more than the relation info then split this out608 if (info !== undefined && info.indexOf('failed') > -1) {
604 if (info.indexOf('relation') > -1) {609 // If we parse more than the relation info then split this out
605 var hook = info.split(':')[1].split('-'),610 if (info.indexOf('relation') > -1) {
606 interfaceName = hook.slice(0, hook.length - 2)[0].trim();611 var stateData = unit.agent_state_data;
607 relationError[unit.service] = interfaceName;612 if (stateData) {
613 var farService = stateData['remote-unit'].split('/')[0];
614 if (farService) {
615 relationError[farService] = stateData.hook;
616 }
608 }617 }
609 }618 }
610 }619 }
611 }620 }
612
613 });621 });
614
615 return [aggregate_map, relationError];622 return [aggregate_map, relationError];
616 },623 },
617624
@@ -626,7 +633,9 @@
626 var previous_unit_count = service.get('unit_count');633 var previous_unit_count = service.get('unit_count');
627 service.set('unit_count', sum);634 service.set('unit_count', sum);
628 service.set('aggregated_status', aggregate[0]);635 service.set('aggregated_status', aggregate[0]);
629 service.set('aggregateRelationError', aggregate[1]);636
637 service.set('relationChangeTrigger', { error: aggregate[1] });
638
630 // Set Google Analytics tracking event.639 // Set Google Analytics tracking event.
631 if (previous_unit_count !== sum && window._gaq) {640 if (previous_unit_count !== sum && window._gaq) {
632 window._gaq.push(['_trackEvent', 'Service Stats', 'Update',641 window._gaq.push(['_trackEvent', 'Service Stats', 'Update',
633642
=== modified file 'app/templates/service-relations-list.handlebars'
--- app/templates/service-relations-list.handlebars 2013-09-19 20:11:53 +0000
+++ app/templates/service-relations-list.handlebars 2013-11-26 20:42:30 +0000
@@ -4,7 +4,7 @@
4</div>4</div>
5{{/unless}}5{{/unless}}
6{{#relations}}6{{#relations}}
7 <h3>{{far.service}}</h3>7 <div class="relation-label {{relationStatus far.service ../errors}}"><h3>{{far.service}}</h3></div>
8 <h4>Interface: {{interface}}</h4>8 <h4>Interface: {{interface}}</h4>
9 <h4>Name: {{near.name}}</h4>9 <h4>Name: {{near.name}}</h4>
10 <h4>Role: {{near.role}}</h4>10 <h4>Role: {{near.role}}</h4>
1111
=== modified file 'app/templates/service-relations-viewlet.handlebars'
--- app/templates/service-relations-viewlet.handlebars 2013-09-16 20:09:02 +0000
+++ app/templates/service-relations-viewlet.handlebars 2013-11-26 20:42:30 +0000
@@ -1,4 +1,4 @@
1<div class="view-container settings-config">1<div class="view-container settings-config">
2 <h2>Relations</h2>2 <h2>Relations</h2>
3 <div data-bind="aggregateRelations"></div>3 <div data-bind="relationChangeTrigger"></div>
4</div>4</div>
55
=== removed file 'app/templates/service-relations.handlebars'
--- app/templates/service-relations.handlebars 2013-04-11 16:16:12 +0000
+++ app/templates/service-relations.handlebars 1970-01-01 00:00:00 +0000
@@ -1,40 +0,0 @@
1{{> service-header}}
2<div class="view-container">
3 <div>
4 <h5>Service Relations</h5>
5 <form id="service-relations">
6 <table class="table table-striped table-bordered">
7 <thead>
8 <tr>
9 <th>Id</th>
10 <th>Role</th>
11 <th>Remote</th>
12 <th>Scope</th>
13 </thead>
14
15 <tbody>
16 {{#relations}}
17 <tr {{#if highlight}}class="highlighted"{{/if}}>
18 <td>
19 {{ident}}
20 <button type="submit" class="btn" id="{{elementId}}" value="{{relation_id}}">
21 Remove
22 </button>
23 </td>
24 <td>{{near.role}}</td>
25 <td>
26 {{#if far}}
27 <a href="{{../../serviceRemoteUri}}{{far.service}}/">{{far.service}}</a>
28 {{/if}}
29 </td>
30 <td>{{scope}}</td>
31 </tr>
32 {{/relations}}
33 </tbody>
34 </table>
35 </form>
36 </div>
37</div>
38<div id="remove-modal-panel"></div>
39{{> service-footer}}
40
410
=== modified file 'app/views/utils.js'
--- app/views/utils.js 2013-11-21 22:42:48 +0000
+++ app/views/utils.js 2013-11-26 20:42:30 +0000
@@ -1686,6 +1686,12 @@
1686 /*jshint debug:false */1686 /*jshint debug:false */
1687 });1687 });
16881688
1689 Y.Handlebars.registerHelper('relationStatus', function(value, errors) {
1690 if (errors[value]) {
1691 return 'error';
1692 }
1693 });
1694
1689 /*1695 /*
1690 * Extension for views to provide an apiFailure method.1696 * Extension for views to provide an apiFailure method.
1691 *1697 *
16921698
=== modified file 'app/views/viewlets/service-relations.js'
--- app/views/viewlets/service-relations.js 2013-09-10 14:53:01 +0000
+++ app/views/viewlets/service-relations.js 2013-11-26 20:42:30 +0000
@@ -33,20 +33,16 @@
33 template: templates['service-relations-viewlet'],33 template: templates['service-relations-viewlet'],
3434
35 bindings: {35 bindings: {
36 aggregateRelations: {36 relationChangeTrigger: {
37 'update': function(node, value) {37 'update': function(node, value) {
38 var db = this.viewlet.options.db;38 var db = this.viewlet.options.db;
39 var service = this.viewlet.model;39 var service = this.viewlet.model;
40 var relations = utils.getRelationDataForService(db, service);40 var relations = utils.getRelationDataForService(db, service);
41 node.setHTML(templates['service-relations-list']({41 node.setHTML(templates['service-relations-list']({
42 relations: relations42 relations: relations,
43 errors: value && value.error
43 }));44 }));
44 }45 }
45 },
46 aggregateRelationError: {
47 'update': function(node, value) {
48 // Aggregate the unit statuses here to display the relation status
49 }
50 }46 }
51 }47 }
52 };48 };
5349
=== modified file 'lib/views/juju-inspector.less'
--- lib/views/juju-inspector.less 2013-11-25 21:18:53 +0000
+++ lib/views/juju-inspector.less 2013-11-26 20:42:30 +0000
@@ -776,6 +776,17 @@
776 .transition(none);776 .transition(none);
777 }777 }
778 }778 }
779
780 .relation-label {
781 padding-left: 10px;
782 background-repeat: no-repeat;
783 background-position: 10px 17px;
784 background-image: url("/juju-ui/assets/images/inspector-charm-running.png");
785
786 &.error {
787 background-image: url("/juju-ui/assets/images/inspector-charm-error.png");
788 }
789 }
779 }790 }
780 .status {791 .status {
781 display: inline-block;792 display: inline-block;
782793
=== modified file 'test/test_model.js'
--- test/test_model.js 2013-11-05 18:10:05 +0000
+++ test/test_model.js 2013-11-26 20:42:30 +0000
@@ -133,16 +133,26 @@
133 id: 'wordpress/0',133 id: 'wordpress/0',
134 agent_state: 'pending'});134 agent_state: 'pending'});
135 var wp1 = new models.ServiceUnit({135 var wp1 = new models.ServiceUnit({
136 id: 'wordpress/2',
137 agent_state: 'error',
138 agent_state_info: 'hook failed: "db-relation-changed"',
139 agent_state_data: {
140 hook: 'db-relation-changed',
141 'relation-id': 1,
142 'remote-unit': 'mysql/0'
143 }});
144 var wp2 = new models.ServiceUnit({
136 id: 'wordpress/1',145 id: 'wordpress/1',
137 agent_state: 'error'});146 agent_state: 'error',
138 wordpress.get('units').add([wp0, wp1]);147 agent_state_info: 'hook failed: "install"'});
148 wordpress.get('units').add([wp0, wp1, wp2]);
139149
140 assert.deepEqual(mysql.get('units')150 assert.deepEqual(mysql.get('units')
141 .get_informative_states_for_service(mysql),151 .get_informative_states_for_service(mysql),
142 [{'pending': 2}, {}]);152 [{'pending': 2}, {}]);
143 assert.deepEqual(wordpress.get('units')153 assert.deepEqual(wordpress.get('units')
144 .get_informative_states_for_service(wordpress),154 .get_informative_states_for_service(wordpress),
145 [{'pending': 1, 'error': 1}, {}]);155 [{'pending': 1, 'error': 2}, { mysql: 'db-relation-changed'}]);
146 });156 });
147157
148 it('service unit list should update analytics when units are added',158 it('service unit list should update analytics when units are added',
@@ -178,20 +188,22 @@
178 models.RelationList, true);188 models.RelationList, true);
179 });189 });
180190
181 it('relation changes on service update aggregateRelations', function(done) {191 it('relation changes on service update relationChangeTrigger',
182 var service = new models.Service();192 function(done) {
183 var relations = service.get('relations');193 var service = new models.Service();
184 var handler = relations.on(194 var relations = service.get('relations');
185 '*:add', function() {195 var handler = relations.on(
186 // This means that it will update the aggregate196 '*:add', function() {
187 // relations for databinding197 // This means that it will update the aggregate
188 handler.detach();198 // relations for databinding
189 var isObject = yui.Lang.isObject;199 handler.detach();
190 assert.equal(isObject(service.get('aggregateRelations')), true);200 var isObject = yui.Lang.isObject;
191 done();201 assert.equal(
192 });202 isObject(service.get('relationChangeTrigger')), true);
193 relations.add(new models.Relation());203 done();
194 });204 });
205 relations.add(new models.Relation());
206 });
195207
196 it('service unit objects should parse the service name from unit id',208 it('service unit objects should parse the service name from unit id',
197 function() {209 function() {

Subscribers

People subscribed via source and target branches