Close inspector when service is destroyed

Close inspector when service is destroyed

The inspector was never hooked up to react to services
being removed via the delta and were left in the canvas.
This branch has the inspector listen for the service model
switching to a dying state and then destroying the inspector
and the service menu.

https://code.launchpad.net/~hatch/juju-gui/close-inspector-1228410/+merge/187612

(do not edit description out of merge proposal)

[jeff.pihach, 2013-09-25 21:27:26]

Please take a look.

[jeff.pihach, 2013-09-25 21:29:24]

Reviewer comments

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

https://codereview.appspot.com/13938043/diff/1/app/views/environment.js#newco...
app/views/environment.js:156: model.on('lifeChange', function(e) {
Because of the sequence of the deltas when removing a service this needs to
listen to the service being set to a dying state - not being destroyed.

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

https://codereview.appspot.com/13938043/diff/1/app/views/inspector.js#newcode428
app/views/inspector.js:428: if (model.name === 'service' &&
!model.get('pending')) {
driveby fix

[frankban, 2013-09-26 07:55:34]

Thank you Jeff, this branch is nice and definitely fixes one aspect of the bug.
See below my comments about adding tests and above all my QA report.

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

https://codereview.appspot.com/13938043/diff/1/app/models/models.js#newcode388
app/models/models.js:388: // Because the packageName is not set if the
Is this logic tested somewhere?

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

https://codereview.appspot.com/13938043/diff/1/app/views/environment.js#newco...
app/views/environment.js:155: // destroy the inspector and hide the service menu
Minor: please add a final dot to this comment.

Also: do we need to add a test explicitly ensuring that the inspector is closed
when the mega watcher announces that a service is being destroyed?

https://codereview.appspot.com/13938043/diff/1/app/views/environment.js#newco...
app/views/environment.js:156: model.on('lifeChange', function(e) {
On 2013/09/25 21:29:24, jeff.pihach wrote:
> Because of the sequence of the deltas when removing a service this needs to
> listen to the service being set to a dying state - not being destroyed.

I think we should listen on both events. In juju-core, QA is ok if the service
successfully dies when service-destroy is called. But the bug is not fixed in
the case the service goes in an error state. In that case, the service switches
to "dying" but stays around as a zombie until the error is resolved.
If you use the inspector to resolve the service unit, the service block
disappears, but the inspector is left open.

I duped doing the following:

- deploy mysql from the GUI and wait for the unit to be ready/started;
- deploy mediawiki from the GUI;
- while mediawiki is in a pending state, add a db:db relation between the two
services;
- destroy mediawiki;
- after a couple of seconds, you should see an error in mysql: hook failed:
"relation-broken";
- open the mysql inspector;
- from the console, run "juju destroy-service mysql";
- the inspector disappears (the service now is dying) but the service block is
(correctly) still there (the service was not removed);
- reopen the mysql inspector and resolve the unit in error;
- at this point, juju-core sends a remove service delta, so the service block
(correctly) disappears, but the inspector is left open (doh!).

I am ok if you want to tackle this in another card.

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

https://codereview.appspot.com/13938043/diff/1/app/views/inspector.js#newcode428
app/views/inspector.js:428: if (model.name === 'service' &&
!model.get('pending')) {
On 2013/09/25 21:29:24, jeff.pihach wrote:
> driveby fix

Thank you!

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

https://codereview.appspot.com/13938043/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:273: assert.isTrue(removeServiceCalled);
Perhaps you meant destroyServiceCalled here?

[jeff.pihach, 2013-09-27 19:13:40]

Please take a look.

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

https://codereview.appspot.com/13938043/diff/1/app/models/models.js#newcode388
app/models/models.js:388: // Because the packageName is not set if the
As a side effect of deploying a service and ghost both paths are tested.

[jeff.pihach, 2013-09-27 19:19:24]

updated reviewer comments

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js#newco...
app/models/models.js:82: instance.destroy();
Whenever we remove a service from a list we now destroy it, application wide -
this is so that anything which relies on that service knows when juju says it
doesn't exist anymore.

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js#newco...
app/models/models.js:556: units.fire('deltaChange', { service: service });
Because units are POJO's they don't fire change events so we need to fire one
manually

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

https://codereview.appspot.com/13938043/diff/10001/app/views/environment.js#n...
app/views/environment.js:154: model.get('units').on('deltaChange', function(e) {
This is a temporary fix for a longer term implementation. These two listeners
are essentially listening for every potential combination of console/gui juju
interactions for removing the services to be able to also clear the UI elements
like inspector and menu.

[gary.poster, 2013-09-27 19:46:37]

LGTM with rambles.  Thank you!

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js#newco...
app/models/models.js:82: instance.destroy();
On 2013/09/27 19:19:25, jeff.pihach wrote:
> Whenever we remove a service from a list we now destroy it, application wide -
> this is so that anything which relies on that service knows when juju says it
> doesn't exist anymore.

Cool.  This sounds like it is worth a comment in the code itself, if you are
willing.

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js#newco...
app/models/models.js:556: units.fire('deltaChange', { service: service });
On 2013/09/27 19:19:25, jeff.pihach wrote:
> Because units are POJO's they don't fire change events so we need to fire one
> manually 

Nice! we could include the units themselves too, or just their identifiers? 
Maybe for later.

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

https://codereview.appspot.com/13938043/diff/10001/app/views/environment.js#n...
app/views/environment.js:164: if (started && service.get('life') === 'dying') {
I thought you weren't doing this part?  We won't do it in the future, and it
doesn't really solve anything now?

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

https://codereview.appspot.com/13938043/diff/10001/app/views/inspector.js#new...
app/views/inspector.js:428: if (model.name === 'service' &&
!model.get('pending')) {
Instead of adding this condition you could just add the next block ("if
(model.get('pending')) {") before this one.  <shrug>

if (model.get('pending')) {
   ...
} else if (model.name === 'service') {
   ...
} else { ... }


Alternatively, I thought we ripping out the destroy behavior in this context...

  433         db.services.remove(model);
  434         model.destroy();

https://codereview.appspot.com/13938043/diff/10001/app/views/inspector.js#new...
app/views/inspector.js:466: service.destroy();
Again, I thought we were ripping this out.  Maybe we should wait for Luca,
yeah...

[jeff.pihach, 2013-09-27 20:13:38]

Thanks for the review - comments below.

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js#newco...
app/models/models.js:82: instance.destroy();
On 2013/09/27 19:46:37, gary.poster wrote:
> On 2013/09/27 19:19:25, jeff.pihach wrote:
> > Whenever we remove a service from a list we now destroy it, application wide
-
> > this is so that anything which relies on that service knows when juju says
it
> > doesn't exist anymore.
> 
> Cool.  This sounds like it is worth a comment in the code itself, if you are
> willing.

Done.

https://codereview.appspot.com/13938043/diff/10001/app/models/models.js#newco...
app/models/models.js:556: units.fire('deltaChange', { service: service });
I was thinking about that, but they are readily available to anything which can
listen for this event where not necessarily for the service.

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

https://codereview.appspot.com/13938043/diff/10001/app/views/environment.js#n...
app/views/environment.js:164: if (started && service.get('life') === 'dying') {
This catches one of the cases where a service is dying and a unit changes from
error to running. If this check isn't here it will then hide the inspector every
time a unit is running properly

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

https://codereview.appspot.com/13938043/diff/10001/app/views/inspector.js#new...
app/views/inspector.js:466: service.destroy();
They are being destroyed after they are removed from the list - so the model at
this point would be in limbo waiting to be GC'd by the browser. It's now being
intentionally destroyed so that anywhere that was using that model knows it's
gone without having to rely on any delta stuff.

[jeff.pihach, 2013-09-27 20:31:11]

*** Submitted:

Close inspector when service is destroyed

The inspector was never hooked up to react to services
being removed via the delta and were left in the canvas.
This branch has the inspector listen for the service model
switching to a dying state and then destroying the inspector
and the service menu.

R=frankban, gary.poster
CC=
https://codereview.appspot.com/13938043