Merge lp:~jcsackett/juju-gui/charm-unification-cleanup into lp:juju-gui/experimental

[j.c.sackett]

Minor clean ups from model unification.

* XXX notes have been consolidated to indicate what should actually be done once
possible.
* The checks in BrowserCharm.parse have been updated based on investigation into
the PyJuju and GoJuju environments. As the data passed is consistently in one
form or another, we now just check if "config" exists and parse
the data as needed in that case.

https://codereview.appspot.com/12897043/

[j.c.sackett (jcsackett)]

Reviewers: mp+180008_code.launchpad.net,

Message:
Please take a look.

Description:
Minor clean ups from model unification.

This began as an effort to do some cleanup from the charm unification
work.
However, the real cleanup consists of removing models.Charm, which we
have
elected to not do at this time, and removing the Charm View code (and
related
attributes on BrowserCharm). The latter cannot be done until the Service
Inspector is no longer feature flagged.

* XXX notes have been consolidated to indicate what should actually be
done once
possible.
* The checks in BrowserCharm.parse have been updated based on
investigation into
the PyJuju and GoJuju environments:
* * Go Juju and Py Juju both pass "subordinate", "config", and
"requires/provides".
* * Fake backend doesn't actually pass this information in at all, as
charms only
reach parse on deploy from the charm browser.
* * Charm browser passes in the browsercharm attributes, e.g.
"is_subordinate".
* * As the data is consistent, we now just check if "config" exists and
parse
the data as needed in that case.

https://code.launchpad.net/~jcsackett/juju-gui/charm-unification-cleanup/+merge/180008

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/models/charm.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/charm.js
=== modified file 'app/models/charm.js'
--- app/models/charm.js 2013-08-12 17:39:36 +0000
+++ app/models/charm.js 2013-08-13 21:34:51 +0000
@@ -111,6 +111,9 @@
     *
     * @class Charm
     */
+ // XXX jcsackett Aug 12 2013 Charm model is only being kept while we
observe
+ // the effects of the changeover to Browsercharm. This can be deleted
once we
+ // ascertain there is no fallout.
    var Charm = Y.Base.create('charm', Y.Model, [], {

      /**
@@ -148,8 +151,6 @@
       * @param {Object} cfg The configuration object.
       */
      initializer: function(cfg) {
- // XXX jcsackett July 19 2013 This is temporary while resolving
Charm and
- // BrowserCharm; Charm wants a fully qualified url as it's ID.
        if (cfg && cfg.url) {
          this.set('id', cfg.url);
        }
@@ -164,10 +165,6 @@
        Y.Object.each(
            parts,
            function(value, key) { self.set(key, value); });
- // XXX jcsackett July 16 2013 There are a raft of bits and bobs of
- // differences between the two charm models that need to be resolved
for
- // the new API to work with the old model. These will no longer be
needed
- // when we switch over to BrowserCharm everywhere.
        if (cfg) {
          if (cfg.config) {
            this.set('options', cfg.config.options);
@@ -210,12 +207,7 @@
        var data = Charm.superclass.parse.apply(this, arguments),
            self = this;

- // TODO (gary): verify whether is_subordinate is ever...

[lp:~jcsackett/juju-gui/charm-unification-cleanup]

951. By j.c.sackett on 2013-08-14

Merged trunk.

[j.c.sackett (jcsackett)]

Please take a look.

https://codereview.appspot.com/12897043/

[Madison Scott-Clary (makyo)]

LGTM, thanks

https://codereview.appspot.com/12897043/

[Richard Harding (rharding)]

LGTM though I don't get parse called on the super class? Can you
verify/double check that?

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

https://codereview.appspot.com/12897043/diff/4001/app/models/charm.js#newcode518
app/models/charm.js:518: var data =
models.BrowserCharm.superclass.parse.apply(this, arguments),
why is this calling it's own method?

https://codereview.appspot.com/12897043/

[j.c.sackett (jcsackett)]

https://codereview.appspot.com/12897043/diff/4001/app/models/charm.js#newcode518
> app/models/charm.js:518: var data =
> models.BrowserCharm.superclass.parse.apply(this, arguments),
> why is this calling it's own method?

It's calling the Models.parse method, I assume. This is a port from what
Charm did.

https://codereview.appspot.com/12897043/

[lp:~jcsackett/juju-gui/charm-unification-cleanup]

952. By j.c.sackett on 2013-08-14

Merged trunk.

953. By j.c.sackett on 2013-08-14

Merged trunk.

[j.c.sackett (jcsackett)]

*** Submitted:

Minor clean ups from model unification.

* XXX notes have been consolidated to indicate what should actually be
done once
possible.
* The checks in BrowserCharm.parse have been updated based on
investigation into
the PyJuju and GoJuju environments. As the data passed is consistently
in one
form or another, we now just check if "config" exists and parse
the data as needed in that case.

R=matthew.scott, rharding
CC=
https://codereview.appspot.com/12897043

https://codereview.appspot.com/12897043/

=== modified file 'app/models/charm.js'
--- app/models/charm.js	2013-08-13 19:01:52 +0000
+++ app/models/charm.js	2013-08-14 21:41:44 +0000
@@ -112,6 +112,9 @@
    *
    * @class Charm
    */
+  // XXX jcsackett Aug 12 2013 Charm model is only being kept while we observe
+  // the effects of the changeover to Browsercharm. This can be deleted once we
+  // ascertain there is no fallout.
   var Charm = Y.Base.create('charm', Y.Model, [], {
 
     /**
@@ -149,8 +152,6 @@
      * @param {Object} cfg The configuration object.
      */
     initializer: function(cfg) {
-      // XXX jcsackett July 19 2013 This is temporary while resolving Charm and
-      // BrowserCharm; Charm wants a fully qualified url as it's ID.
       if (cfg && cfg.url) {
         this.set('id', cfg.url);
       }
@@ -165,10 +166,6 @@
       Y.Object.each(
           parts,
           function(value, key) { self.set(key, value); });
-      // XXX jcsackett July 16 2013 There are a raft of bits and bobs of
-      // differences between the two charm models that need to be resolved for
-      // the new API to work with the old model. These will no longer be needed
-      // when we switch over to BrowserCharm everywhere.
       if (cfg) {
         if (cfg.config) {
           this.set('options', cfg.config.options);
@@ -211,12 +208,7 @@
       var data = Charm.superclass.parse.apply(this, arguments),
           self = this;
 
-      // TODO (gary): 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;
-      // Because the old and new charm models have different places for
-      // the options data, this handles the normalization.
       if (data.config && data.config.options && !data.options) {
         data.options = data.config.options;
         delete data.config;
@@ -264,10 +256,6 @@
         }
       },
       bzr_branch: {},
-      //XXX jcsackett July 31 2013 This attribute is only needed until we turn
-      // on the service inspector. It's just used by the charm view you get when
-      // inspecting a service, and should be ripped out (along with tests) when
-      // we remove that view.
       charm_path: {
         /**
          * Generate the charm store path from the attributes of the charm.
@@ -528,28 +516,23 @@
     },
 
     parse: function(response) {
-      var data = Charm.superclass.parse.apply(this, arguments),
+      var data = models.BrowserCharm.superclass.parse.apply(this, arguments),
           self = this;
 
-      // TODO (gary): 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;
-      // Because the old and new charm models have different places for
-      // the options data, this handles the normalization.
-
-      // TODO (jcsackett): Verify whether parse *ever* gets data.config or
-      // data.relations, if not we can remove the checks and default to altering
-      // the data.
-      if (data.config && data.config.options && !data.options) {
+      //Data can come from two places; a BrowserCharm being deployed into the
+      //environment, or a charm already in the environment. They have slightly
+      //different attributes.
+      if (data.config) {
+        // If data has a 'config' attribute, we're dealing with data from the
+        // environment.
         data.options = data.config.options;
-        delete data.config;
-      }
-      if (!data.relations && (data.requires || data.provides)) {
         data.relations = {
           requires: data.requires,
           provides: data.provides
         };
+        data.is_subordinate = data.subordinate;
+        delete data.config;
+        delete data.subordinate;
         delete data.requires;
         delete data.provides;
       }