Merge into trunk : bug1204331 : Code : juju-gui

Status:	Merged
Merged at revision:	893
Proposed branch:	lp://staging/~gary/juju-gui/bug1204331
Merge into:	lp://staging/juju-gui/experimental
Diff against target:	208 lines (+64/-11) 10 files modified app/index.html (+2/-2) app/models/charm.js (+4/-1) app/store/endpoints.js (+2/-0) app/templates/ghost-config-wrapper.handlebars (+2/-0) app/views/charm-panel.js (+2/-1) app/views/environment.js (+2/-1) app/views/ghost-inspector.js (+7/-3) app/views/inspector.js (+6/-3) app/views/utils.js (+12/-0) test/test_endpoints.js (+25/-0)
To merge this branch:	bzr merge lp://staging/~gary/juju-gui/bug1204331
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-07-25	Pending
Review via email: mp+176830@code.staging.launchpad.net

Description of the change

Fix subordinate behavior in Juju Core

The biggest change here is that we update service data with the charm subordinate flag, because juju core does not include that information in the service. This includes a test.

Other changes here are fly-by.
- The yui3-skin-sam class on the body was causing issues by adding styles unnecessarily. I put it on the subapp-browser, where it is sufficient.
- The sandbox sends is_subordinate, not subordinate, for charms. I'm not sure if that is correct, but for now I changed charm.js to handle it. I'd welcome opinions on pushing this back down into the pyjuju sandbox, where I could test it. I'm kind of inclined to do that, actually, but I want to get this branch proposed.
- I made a few changes to the inspector code to handle subordinates a bit better, particularly in the ghost inspector. the ghost inspector has no tests at all, and we have a card for adding them, so I did not try to tack that effort on to this branch.
- I made a small tweak to charm-panel.js so that new services start looking like subordinates immediately.
- I added a small hack that came in handy to track down behavior in a handlebars template. Just add {{debugger}} in the template and you will have a breakpoint there. handy. hopefully not too dangerous! We definitely don't want to check in a template that uses this, and the linter won't check our template code...

https://codereview.appspot.com/11804043/

Revision history for this message

Gary Poster (gary) wrote on 2013-07-25:

#

Download full text (10.0 KiB)

Reviewers: mp+176830_code.launchpad.net,

Message:
Please take a look.

Description:
Fix subordinate behavior in Juju Core

The biggest change here is that we update service data with the charm
subordinate flag, because juju core does not include that information in
the service. This includes a test.

Other changes here are fly-by.
- The yui3-skin-sam class on the body was causing issues by adding
styles unnecessarily. I put it on the subapp-browser, where it is
sufficient.
- The sandbox sends is_subordinate, not subordinate, for charms. I'm
not sure if that is correct, but for now I changed charm.js to handle
it. I'd welcome opinions on pushing this back down into the pyjuju
sandbox, where I could test it. I'm kind of inclined to do that,
actually, but I want to get this branch proposed.
- I made a few changes to the inspector code to handle subordinates a
bit better, particularly in the ghost inspector. the ghost inspector
has no tests at all, and we have a card for adding them, so I did not
try to tack that effort on to this branch.
- I made a small tweak to charm-panel.js so that new services start
looking like subordinates immediately.
- I added a small hack that came in handy to track down behavior in a
handlebars template. Just add {{debugger}} in the template and you will
have a breakpoint there. handy. hopefully not too dangerous! We
definitely don't want to check in a template that uses this, and the
linter won't check our template code...

https://code.launchpad.net/~gary/juju-gui/bug1204331/+merge/176830

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/index.html
   M app/models/charm.js
   M app/store/endpoints.js
   M app/templates/ghost-config-wrapper.handlebars
   M app/views/charm-panel.js
   M app/views/environment.js
   M app/views/ghost-inspector.js
   M app/views/inspector.js
   M app/views/utils.js
   M test/test_endpoints.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/index.html
=== modified file 'app/index.html'
--- app/index.html 2013-07-24 14:30:06 +0000
+++ app/index.html 2013-07-24 21:46:12 +0000
@@ -67,7 +67,7 @@
</script>
</head>

- <body class="yui3-skin-sam">
+ <body>
        
@@ -164,7 +164,7 @@
            <div id="content">
                <div id="shortcut-help" style="display:none"></div>
                <div id="subapp-browser-min" style="display: none;"></div>
- <div id="subapp-browser"></div>
+ <div id="subapp-browser" class="yui3-skin-sam"></div>
                <div id="main">
                </div> 
           ...

Reviewers: mp+176830_code.launchpad.net,

Message:
Please take a look.

Description:
Fix subordinate behavior in Juju Core

The biggest change here is that we update service data with the charm
subordinate flag, because juju core does not include that information in
the service.  This includes a test.

Other changes here are fly-by.
- The yui3-skin-sam class on the body was causing issues by adding
styles unnecessarily.  I put it on the subapp-browser, where it is
sufficient.
- The sandbox sends is_subordinate, not subordinate, for charms.  I'm
not sure if that is correct, but for now I changed charm.js to handle
it.  I'd welcome opinions on pushing this back down into the pyjuju
sandbox, where I could test it.  I'm kind of inclined to do that,
actually, but I want to get this branch proposed.
- I made a few changes to the inspector code to handle subordinates a
bit better, particularly in the ghost inspector.  the ghost inspector
has no tests at all, and we have a card for adding them, so I did not
try to tack that effort on to this branch.
- I made a small tweak to charm-panel.js so that new services start
looking like subordinates immediately.
- I added a small hack that came in handy to track down behavior in a
handlebars template.  Just add {{debugger}} in the template and you will
have a breakpoint there.  handy.  hopefully not too dangerous!  We
definitely don't want to check in a template that uses this, and the
linter won't check our template code...

https://code.launchpad.net/~gary/juju-gui/bug1204331/+merge/176830

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/index.html
   M app/models/charm.js
   M app/store/endpoints.js
   M app/templates/ghost-config-wrapper.handlebars
   M app/views/charm-panel.js
   M app/views/environment.js
   M app/views/ghost-inspector.js
   M app/views/inspector.js
   M app/views/utils.js
   M test/test_endpoints.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: jeff.pihach@canonical.com-20130724190022-ml45mqciysgohor5
+New revision: gary.poster@canonical.com-20130725004331-2luvah1uam8zd230

Index: app/index.html
=== modified file 'app/index.html'
--- app/index.html	2013-07-24 14:30:06 +0000
+++ app/index.html	2013-07-24 21:46:12 +0000
@@ -67,7 +67,7 @@
      </script>
    </head>

-  <body class="yui3-skin-sam">
+  <body>
        
@@ -164,7 +164,7 @@
            <div id="content">
                <div id="shortcut-help" style="display:none"></div>
                <div id="subapp-browser-min" style="display: none;"></div>
-              <div id="subapp-browser"></div>
+              <div id="subapp-browser" class="yui3-skin-sam"></div>
                <div id="main">
                </div> 
            </div>

Index: app/models/charm.js
=== modified file 'app/models/charm.js'
--- app/models/charm.js	2013-07-19 20:22:52 +0000
+++ app/models/charm.js	2013-07-24 14:29:33 +0000
@@ -173,7 +173,10 @@
      parse: function() {
        var data = Charm.superclass.parse.apply(this, arguments),
            self = this;
-      data.is_subordinate = data.subordinate;
+      // TODO: verify whether is_subordinate is ever passed by pyjuju or  
juju
+      // core.  If not, remove the "|| data.is_subordinate" and change in  
the
+      // fakebackend and/or sandbox to send the expected thing there.
+      data.is_subordinate = data.subordinate || data.is_subordinate;
        Y.each(data, function(value, key) {
          if (!value ||
              !self.attrAdded(key) ||

Index: app/store/endpoints.js
=== modified file 'app/store/endpoints.js'
--- app/store/endpoints.js	2013-05-17 14:51:05 +0000
+++ app/store/endpoints.js	2013-07-24 14:29:33 +0000
@@ -141,6 +141,8 @@

servicePromise.then(
                  function(data) {
+                  data.service.set(
+                      'subordinate', data.charm.get('is_subordinate'));
                    self.addServiceToEndpointsMap(
                        data.service.get('id'), data.charm);
                  },

Index: app/templates/ghost-config-wrapper.handlebars
=== modified file 'app/templates/ghost-config-wrapper.handlebars'
--- app/templates/ghost-config-wrapper.handlebars	2013-07-22 19:42:33 +0000
+++ app/templates/ghost-config-wrapper.handlebars	2013-07-24 21:46:12 +0000
@@ -6,6 +6,7 @@
        <input value="Confirm" type="submit"
               class="btn btn-primary deploy">
      </div>
+    {{#unless isSubordinate}}
      <div class="charm-entry">
        <div class="control-group">
          <div class="well control-description">
@@ -18,6 +19,7 @@
          </div>
        </div>
      </div>
+    {{/unless}}
    </div>
    <div class="viewlet-container"></div>
    <div class="viewlet-manager-footer"></div>

Index: app/views/charm-panel.js
=== modified file 'app/views/charm-panel.js'
--- app/views/charm-panel.js	2013-07-24 03:22:41 +0000
+++ app/views/charm-panel.js	2013-07-24 14:29:33 +0000
@@ -274,7 +274,8 @@
                charm: charm.get('id'),
                unit_count: 0,  // No units yet.
                loaded: false,
-              config: options
+              config: options,
+              subordinate: charm.get('is_subordinate')
              });
              // If we have been given coordinates at which the ghost should  
be
              // created, respect them.

Index: app/views/environment.js
=== modified file 'app/views/environment.js'
--- app/views/environment.js	2013-07-22 15:15:51 +0000
+++ app/views/environment.js	2013-07-24 21:46:12 +0000
@@ -240,7 +240,8 @@
                // the configuration for the view manager template
                templateConfig: {
                  packageName: model.get('package_name'),
-                id: model.get('id')
+                id: model.get('id'),
+                isSubordinate: model.get('is_subordinate')
                }
              }
            };

Index: app/views/ghost-inspector.js
=== modified file 'app/views/ghost-inspector.js'
--- app/views/ghost-inspector.js	2013-07-20 00:08:41 +0000
+++ app/views/ghost-inspector.js	2013-07-24 21:46:12 +0000
@@ -94,9 +94,13 @@
      deployCharm: function() {
        var options = this.options,
            container = options.container,
+          model = options.model,
            serviceName =  
container.one('input[name=service-name]').get('value'),
-          numUnits = parseInt(
-              container.one('input[name=number-units]').get('value'), 10),
+          isSubordinate = model.get('is_subordinate'),
+          numUnits = (
+              isSubordinate ? 0 :
+              parseInt(
+                  container.one('input[name=number-units]').get('value'),  
10)),
            config;

if (this.checkForExistingService(serviceName)) {
@@ -118,7 +122,7 @@
        }

options.env.deploy(
-          this.options.model.get('id'),
+          model.get('id'),
            serviceName, config, this.configFileContent,
            numUnits, Y.bind(
                this._deployCallbackHandler, this, serviceName, config));

Index: app/views/inspector.js
=== modified file 'app/views/inspector.js'
--- app/views/inspector.js	2013-07-24 15:40:51 +0000
+++ app/views/inspector.js	2013-07-24 21:46:12 +0000
@@ -1066,9 +1066,12 @@
            units: {
              depends: ['aggregated_status'],
              'update': function(node, value) {
-              // called under the databinding context
-              var statuses = this.viewlet.updateUnitList(value);
-              this.viewlet.generateAndBindUnitHeaders(node, statuses);
+              // Called under the databinding context.
+              // Subordinates may not have a value.
+              if (value) {
+                var statuses = this.viewlet.updateUnitList(value);
+                this.viewlet.generateAndBindUnitHeaders(node, statuses);
+              }
              }
            }
          },

Index: app/views/utils.js
=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-07-24 14:30:06 +0000
+++ app/views/utils.js	2013-07-25 00:43:31 +0000
@@ -1427,6 +1427,18 @@
        return options.fn(this);
      }
    });
+
+  /*
+   * Dev tool: dump to debugger in template.
+   *
+   * {{debugger}}
+   *
+   */
+  Y.Handlebars.registerHelper('debugger', function() {
+    /*jshint debug:true */
+    debugger;
+    /*jshint debug:false */
+  });
    /*
     * Extension for views to provide an apiFailure method.
     *

Index: test/test_endpoints.js
=== modified file 'test/test_endpoints.js'
--- test/test_endpoints.js	2013-06-19 15:44:25 +0000
+++ test/test_endpoints.js	2013-07-25 00:43:31 +0000
@@ -393,8 +393,33 @@

var svc = app.db.services.getById(service_name);
         svc.set('pending', false);
+       destroyMe.push(svc);
       });

+  it('should update is_subordinate value when non-pending services are  
added',
+      function(done) {
+        var service_name = 'puppet';
+        var charm_id = 'cs:precise/puppet-2';
+        app.db.charms.add({id: charm_id, is_subordinate: true});
+        var charm = app.db.charms.getById(charm_id);
+        destroyMe.push(charm);
+        charm.loaded = true;
+        app.db.services.add({
+          id: service_name,
+          pending: true,
+          loaded: true,
+          charm: charm_id});
+
+        controller.on('endpointMapAdded', function() {
+          assert.isTrue(svc.get('subordinate'));
+          done();
+        });
+
+        var svc = app.db.services.getById(service_name);
+        svc.set('pending', false);
+        destroyMe.push(svc);
+      });
+
    it('should update endpoints map when a service\'s charm changes',  
function() {
      var service_name = 'wordpress';
      var charm_id = 'cs:precise/wordpress-2';

Revision history for this message

Brad Crittenden (bac) wrote on 2013-07-25:

#

LGTM for code. QA shows the dismiss [X] on the inspector has dropped
50px or so out of place. The issues you meant to solve are fixed.

https://codereview.appspot.com/11804043/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/11804043/diff/1/app/index.html#newcode70
app/index.html:70: <body>
I'm glad you resolved this issue.

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

https://codereview.appspot.com/11804043/diff/1/app/models/charm.js#newcode178
app/models/charm.js:178: // fakebackend and/or sandbox to send the
expected thing there.
Trivial but this should be an XXX with your name on it.

https://codereview.appspot.com/11804043/

Revision history for this message

Gary Poster (gary) wrote on 2013-07-25:

#

*** Submitted:

Fix subordinate behavior in Juju Core

The biggest change here is that we update service data with the charm
subordinate flag, because juju core does not include that information in
the service. This includes a test.

Other changes here are fly-by.
- The yui3-skin-sam class on the body was causing issues by adding
styles unnecessarily. I put it on the subapp-browser, where it is
sufficient.
- The sandbox sends is_subordinate, not subordinate, for charms. I'm
not sure if that is correct, but for now I changed charm.js to handle
it. I'd welcome opinions on pushing this back down into the pyjuju
sandbox, where I could test it. I'm kind of inclined to do that,
actually, but I want to get this branch proposed.
- I made a few changes to the inspector code to handle subordinates a
bit better, particularly in the ghost inspector. the ghost inspector
has no tests at all, and we have a card for adding them, so I did not
try to tack that effort on to this branch.
- I made a small tweak to charm-panel.js so that new services start
looking like subordinates immediately.
- I added a small hack that came in handy to track down behavior in a
handlebars template. Just add {{debugger}} in the template and you will
have a breakpoint there. handy. hopefully not too dangerous! We
definitely don't want to check in a template that uses this, and the
linter won't check our template code...

R=benji
CC=
https://codereview.appspot.com/11804043

https://codereview.appspot.com/11804043/

juju-gui

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

Commit message

Description of the change

Preview Diff

Subscribers