Separate environment and store charms

Separate environment and store charms

On discussion with Kapil, a significant problem from a previous refactoring of mine became clear.  Charm ids are only reliably unique within a given context, such as juju environment vs charm store.  Therefore, charms from the two sources need to be stored separately.

We discussed some approaches.  He suggested storing environment charms in the database and store charms in the browser, and I liked that idea.  He also requested that I factor out the charm model code into a separate file, even while keeping it in the juju.models package.  Finally, he suggested that charm ids should always include revisions in order to guarantee uniqueness, and to make it possible to consider whether charms from the different sources are identical.

I wrote a long explanation of this in the app/models/charms.js file, including additional considerations.

The charm store needed to send the revision itself, which Kapil changed it to do.

I ended up also factoring out a charm store object, with the ability to get the data from a search, organized as we like it; and to get the data for a specific charm.  A nice fall out from the tests of this code is that it exposed some pre-existing problems with sorting code, which I addressed.

The change is quite large because it touches so many of the files.  On the bright side, many of the test changes are mechanical, and much of the code is moved and only slightly refactored and changed from other sources, so deletions and moves account for much of the churn.

Kapil made some great changes to the charm store after reviewing this branch that will let us simplify this branch's code in a follow on.

https://code.launchpad.net/~gary/juju-gui/charmdivision/+merge/130464

(do not edit description out of merge proposal)

[gary.poster, 2012-10-19 02:11:04]

Please take a look.

[hazmat, 2012-10-19 19:02:34]

The branch looks good. I'm concerned a bit about the ambiguity and work we're
carrying forward to support that. I've update the charm browser to always return
store_url as part of search results as an unambigious qualifier. results without
a store_url are filtered by the server. Also we can now retrieve charm details
with a revision identifier (the backend doesn't do anything with this revision
at the moment, but we can fix that on the backend later). I think that should
make some of the bits here a bit simpler (baseId vs id).

Also with that i'd like to drop the extraneous information we're carrying in the
search results list. ie.. given

    {
      "data_url": "/charms/precise/lamp/json", 
      "name": "lamp", 
      "store_url": "cs:precise/lamp-1", 
      "series": "precise", 
      "summary": "set up an apache server with php and supports a connection to
mysql", 
      "relevance": 19.82845289740461, 
      "owner": "charmers"
    }, 

name/series/owner/data_url keys can all be obsoleted.

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

https://codereview.appspot.com/6749046/diff/1/app/app.js#newcode617
app/app.js:617: 'juju-charm-store']
i think this can get rolled up into models in modules

[gary.poster, 2012-10-20 14:49:27]

*** Submitted:

Separate environment and store charms

On discussion with Kapil, a significant problem from a previous refactoring of
mine became clear.  Charm ids are only reliably unique within a given context,
such as juju environment vs charm store.  Therefore, charms from the two sources
need to be stored separately.

We discussed some approaches.  He suggested storing environment charms in the
database and store charms in the browser, and I liked that idea.  He also
requested that I factor out the charm model code into a separate file, even
while keeping it in the juju.models package.  Finally, he suggested that charm
ids should always include revisions in order to guarantee uniqueness, and to
make it possible to consider whether charms from the different sources are
identical.

I wrote a long explanation of this in the app/models/charms.js file, including
additional considerations.

The charm store needed to send the revision itself, which Kapil changed it to
do.

I ended up also factoring out a charm store object, with the ability to get the
data from a search, organized as we like it; and to get the data for a specific
charm.  A nice fall out from the tests of this code is that it exposed some
pre-existing problems with sorting code, which I addressed.

The change is quite large because it touches so many of the files.  On the
bright side, many of the test changes are mechanical, and much of the code is
moved and only slightly refactored and changed from other sources, so deletions
and moves account for much of the churn.

Kapil made some great changes to the charm store after reviewing this branch
that will let us simplify this branch's code in a follow on.

R=hazmat
CC=
https://codereview.appspot.com/6749046

[gary.poster, 2012-10-20 14:54:52]

On 2012/10/19 19:02:34, hazmat wrote:
> The branch looks good. I'm concerned a bit about the ambiguity and work we're
> carrying forward to support that. I've update the charm browser to always
return
> store_url as part of search results as an unambigious qualifier. results
without
> a store_url are filtered by the server. Also we can now retrieve charm details
> with a revision identifier (the backend doesn't do anything with this revision
> at the moment, but we can fix that on the backend later). I think that should
> make some of the bits here a bit simpler (baseId vs id).
> 
> Also with that i'd like to drop the extraneous information we're carrying in
the
> search results list. ie.. given
> 
>     {
>       "data_url": "/charms/precise/lamp/json", 
>       "name": "lamp", 
>       "store_url": "cs:precise/lamp-1", 
>       "series": "precise", 
>       "summary": "set up an apache server with php and supports a connection
to
> mysql", 
>       "relevance": 19.82845289740461, 
>       "owner": "charmers"
>     }, 
> 
> name/series/owner/data_url keys can all be obsoleted.

That all sounds great--except maybe data_url.  I think keeping that might be a
nice convenience.  But you're still right, even that is arguably duplicate--as
long as we keep the same URL patterns in the charm store.

> 
> https://codereview.appspot.com/6749046/diff/1/app/app.js
> File app/app.js (right):
> 
> https://codereview.appspot.com/6749046/diff/1/app/app.js#newcode617
> app/app.js:617: 'juju-charm-store']
> i think this can get rolled up into models in modules

The charm and charm list are part of models.  the charm store is not--it
represents interactions with the charm store and returns data, not models.  We
*could* collapse it with env if you wanted.  I didn't do it because I didn't see
a win or a real conceptual connection with the juju environment, but I wouldn't
object.

Thanks!

Gary