Inspector Factory & Single Inspector

Inspector Factory & Single Inspector

Restricted the user to a single inspector but it
is done in a way which is trivial to remove or 
wrap a flag around at a later date if we decide 
to change it back. Also there is a factory function
in the environment view to generate new service inspectors.

https://code.launchpad.net/~hatch/juju-gui/single-inspector/+merge/171935

(do not edit description out of merge proposal)

[jeff.pihach, 2013-06-28 00:16:23]

Please take a look.

[jeff.pihach, 2013-06-28 04:14:11]

Please take a look.

[gary.poster, 2013-06-28 13:11:54]

Hi Jeff.  In general I like this a lot.  Nice cleanups.  Before I approve, I'd
like you to look at my two-ghost-service-running-the-same-charm question.

Thanks!

Gary

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

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:113: // the charm so that's what this switcheroo is
doing here.
Nice simple fix, thanks

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:125: var serviceInspector =
this.getInspector(model.get('id'));
hm.  does this mean that you can't have two ghost services running the same
charm?  That would be a bug that needs to be addressed.

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:138: console.log('Service inspector type not
supported.');
It would be interesting to use Google Analytics to send errors like this for us
to review.  Not for this branch, just a thought.

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:147: this.setInspector(e.currentTarget, true);
comment as to motive would help me read this.

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

https://codereview.appspot.com/10724043/diff/4001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:54: // XXX - Jeff 25/06/2013
I think you are right that it is no longer required, FWIW.

[jeff.pihach, 2013-06-28 14:42:09]

Thanks for the review - fix coming

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

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:113: // the charm so that's what this switcheroo is
doing here.
was contemplating a 'there be dragons' warning here somewhere :)

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:125: var serviceInspector =
this.getInspector(model.get('id'));
Correct this limits that - working on fix now.

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:138: console.log('Service inspector type not
supported.');
Agreed!

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:147: this.setInspector(e.currentTarget, true);
On 2013/06/28 13:11:54, gary.poster wrote:
> comment as to motive would help me read this.

Done.

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

https://codereview.appspot.com/10724043/diff/4001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:54: // XXX - Jeff 25/06/2013
I'll remove it and see if anything blows up.

[benjamin.saller, 2013-06-28 17:07:16]

Thanks for this, comments follow. 

I think this is pretty close.

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

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:116: config.ghostService = model;
You do this dance explicitly when calling this method, I think its fine to do it
only here.

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:125: var serviceInspector =
this.getInspector(model.get('id'));
On 2013/06/28 14:42:09, jeff.pihach wrote:
> Correct this limits that - working on fix now.

They would have to have different ids computed to render in the first place. I
think ghostDeploy does this properly.

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:138: console.log('Service inspector type not
supported.');
On 2013/06/28 13:11:54, gary.poster wrote:
> It would be interesting to use Google Analytics to send errors like this for
us
> to review.  Not for this branch, just a thought.

This particular error goes away if we sniff the type from the model. But point
taken, almost any console.log could potentially become a custom event.

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

https://codereview.appspot.com/10724043/diff/4001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:59: this.db.charms.add(charm);
It might make sense to drop the loaded flag here, add the charm and have a
handler on the charms list that pull this in the background. Not sure, but I
think we all know the current pattern is pretty messy.

https://codereview.appspot.com/10724043/diff/4001/app/views/ghost-inspector.j...
app/views/ghost-inspector.js:66: ghostService: ghostService
I'd omit 'ghost' here as mentioned elsewhere, the primary model would be the
ghostService and it will sniff the pending value. Not sure why charm is primary
here.

https://codereview.appspot.com/10724043/diff/4001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/10724043/diff/4001/app/views/topology/service....
app/views/topology/service.js:1356: createServiceInspector('service', service);
Do we need the type 'service' here at all if the method sniffs it from the
pending flag? I'd omit this from the signature.

[jeff.pihach, 2013-06-28 17:42:26]

Thanks for the review - updated branch coming shortly

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

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:116: config.ghostService = model;
On 2013/06/28 17:07:17, benjamin.saller wrote:
> You do this dance explicitly when calling this method, I think its fine to do
it
> only here.

Done.

https://codereview.appspot.com/10724043/diff/4001/app/views/environment.js#ne...
app/views/environment.js:138: console.log('Service inspector type not
supported.');
On 2013/06/28 17:07:17, benjamin.saller wrote:
> On 2013/06/28 13:11:54, gary.poster wrote:
> > It would be interesting to use Google Analytics to send errors like this for
> us
> > to review.  Not for this branch, just a thought.
> 
> This particular error goes away if we sniff the type from the model. But point
> taken, almost any console.log could potentially become a custom event.

Done.

https://codereview.appspot.com/10724043/diff/4001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/10724043/diff/4001/app/views/topology/service....
app/views/topology/service.js:1356: createServiceInspector('service', service);
On 2013/06/28 17:07:17, benjamin.saller wrote:
> Do we need the type 'service' here at all if the method sniffs it from the
> pending flag? I'd omit this from the signature.

Done.

[jeff.pihach, 2013-06-28 17:48:33]

Please take a look.

[benjamin.saller, 2013-06-28 18:20:27]

LGTM, thanks!

[gary.poster, 2013-06-28 18:22:46]

LGTM

[jeff.pihach, 2013-06-28 18:28:50]

*** Submitted:

Inspector Factory & Single Inspector

Restricted the user to a single inspector but it
is done in a way which is trivial to remove or 
wrap a flag around at a later date if we decide 
to change it back. Also there is a factory function
in the environment view to generate new service inspectors.

R=gary.poster, benjamin.saller
CC=
https://codereview.appspot.com/10724043