Ghost inspector now uses service model

Ghost inspector now uses service model

The ghost inspector now uses the service model and only references
the charm model for things like the configuration options 
descriptions.

https://code.launchpad.net/~hatch/juju-gui/ghost-databind/+merge/186837

(do not edit description out of merge proposal)

[jeff.pihach, 2013-09-20 16:21:03]

Please take a look.

[jeff.pihach, 2013-09-20 16:26:28]

Reviewer comments

https://codereview.appspot.com/13334054/diff/1/app/templates/service-configur...
File app/templates/service-configuration.partial (left):

https://codereview.appspot.com/13334054/diff/1/app/templates/service-configur...
app/templates/service-configuration.partial:11: data-bind="{{#if
../../ghost}}options{{else}}config{{/if}}.{{name}}"
Now that we use the same model there is no difference here

https://codereview.appspot.com/13334054/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/13334054/diff/1/app/views/environment.js#newco...
app/views/environment.js:118: model.set('packageName',
charm.get('package_name'));
the package name is added to the service model now so we can revert to it if
they close the inspector while it's in ghost mode

https://codereview.appspot.com/13334054/diff/1/app/views/viewlets/service-gho...
File app/views/viewlets/service-ghost.js (right):

https://codereview.appspot.com/13334054/diff/1/app/views/viewlets/service-gho...
app/views/viewlets/service-ghost.js:43: }
We still need the default values for the config from the charm.

https://codereview.appspot.com/13334054/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/13334054/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:307: destroy_service: function() {
we always go through the environment now to remove services.

[benjamin.saller, 2013-09-20 16:51:28]

Thanks for this, this is clearly the right thing to do. I do have some concerns
though.

I'm not clear on the removal story. Questions included.

Suggestions on display name.

Also a QA oddity. The ghost service id properly indicates conflicting names
except when first allocated. If you deploy ceph and then add another the default
name is ceph but with a green check next to it even though that will fail.

https://codereview.appspot.com/13334054/diff/1/app/templates/service-configur...
File app/templates/service-configuration.partial (left):

https://codereview.appspot.com/13334054/diff/1/app/templates/service-configur...
app/templates/service-configuration.partial:11: data-bind="{{#if
../../ghost}}options{{else}}config{{/if}}.{{name}}"
On 2013/09/20 16:26:28, jeff.pihach wrote:
> Now that we use the same model there is no difference here

Much more sensible

https://codereview.appspot.com/13334054/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/13334054/diff/1/app/views/environment.js#newco...
app/views/environment.js:118: model.set('packageName',
charm.get('package_name'));
On 2013/09/20 16:26:28, jeff.pihach wrote:
> the package name is added to the service model now so we can revert to it if
> they close the inspector while it's in ghost mode

The use of snake and camel cases here will be a fun source of issues for our
future selves.

https://codereview.appspot.com/13334054/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13334054/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:177: this.model.set('displayName', name);
Would it make sense to do this in a getter on the model when pending is set?
Something like?

'getter': function(value) { 
   if (value) return value;
   if (this.get('pending')) return '(' + this.get('id') + ')';
   return this.get('id')
} 

I think we could then bind it normally or maybe set the bindind.depends on
pending. Maybe that is just as much work though.

https://codereview.appspot.com/13334054/diff/1/app/views/inspector.js
File app/views/inspector.js (left):

https://codereview.appspot.com/13334054/diff/1/app/views/inspector.js#oldcode499
app/views/inspector.js:499: } else if (model.name === 'browser-charm') {
When the service is pending we'd still just remove the model, no? It doesn't
exist in the environment.

https://codereview.appspot.com/13334054/diff/1/app/views/viewlets/service-gho...
File app/views/viewlets/service-ghost.js (left):

https://codereview.appspot.com/13334054/diff/1/app/views/viewlets/service-gho...
app/views/viewlets/service-ghost.js:70: // but this will allow us to use the old
template.
Do we still need this comment? the old templates are the only templates now.

[bac, 2013-09-20 16:53:50]

LGTM.  thanks for the helpful comments throughout your changes.  Did not do QA.

https://codereview.appspot.com/13334054/diff/1/app/templates/service-configur...
File app/templates/service-configuration.partial (right):

https://codereview.appspot.com/13334054/diff/1/app/templates/service-configur...
app/templates/service-configuration.partial:11: data-bind="config.{{name}}"
nice

[jeff.pihach, 2013-09-20 17:12:10]

Thanks for the reviews - comments below and fixes coming.

Ben the bug you found exists on comingsoon as well so I'd like to fix in a
follow-up.

https://codereview.appspot.com/13334054/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13334054/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:177: this.model.set('displayName', name);
displayName already has a getter to be able to handle custom set displayNames
and ones coming from Juju.  the name is no longer the 'id' because we need to
generate random id's to avoid the issue of possible name clashes.  So good idea
but probably doesn't get us anything.

https://codereview.appspot.com/13334054/diff/1/app/views/inspector.js
File app/views/inspector.js (left):

https://codereview.appspot.com/13334054/diff/1/app/views/inspector.js#oldcode499
app/views/inspector.js:499: } else if (model.name === 'browser-charm') {
Good find, I'll look into a workaround here because I just introduced a bug.
Thanks

https://codereview.appspot.com/13334054/diff/1/app/views/viewlets/service-gho...
File app/views/viewlets/service-ghost.js (left):

https://codereview.appspot.com/13334054/diff/1/app/views/viewlets/service-gho...
app/views/viewlets/service-ghost.js:70: // but this will allow us to use the old
template.
Not really no, removed.

[jeff.pihach, 2013-09-20 17:16:59]

Please take a look.

[benjamin.saller, 2013-09-20 17:20:13]

LGTM, but didn't QA this round. Thanks for the fix on remove.

https://codereview.appspot.com/13334054/diff/13001/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13334054/diff/13001/app/views/inspector.js#new...
app/views/inspector.js:500: db.services.remove(model);
Thanks

[jeff.pihach, 2013-09-20 17:37:12]

*** Submitted:

Ghost inspector now uses service model

The ghost inspector now uses the service model and only references
the charm model for things like the configuration options 
descriptions.

R=benjamin.saller, bac
CC=
https://codereview.appspot.com/13334054