Fix bug 1166222

Fix bug 1166222

The Go environment is unwilling to load a charm before it is part of the environment, which broke our new endpoints implementation.  Luckily, fixing it ended up being a nice cleanup on our side, I think.  We do not need the charm until it is no longer a ghost.

https://code.launchpad.net/~gary/juju-gui/bug1166222/+merge/158260

(do not edit description out of merge proposal)

[gary.poster, 2013-04-11 01:21:01]

Please take a look.

[teknico, 2013-04-11 09:57:13]

LGTM, nice fix and cleanup. See trivials below.

https://codereview.appspot.com/8647043/diff/1/app/store/endpoints.js
File app/store/endpoints.js (right):

https://codereview.appspot.com/8647043/diff/1/app/store/endpoints.js#newcode156
app/store/endpoints.js:156: */
Why the different style in the handleServiceEvent doc comment definition and
this one?

https://codereview.appspot.com/8647043/diff/1/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8647043/diff/1/test/test_endpoints.js#newcode330
test/test_endpoints.js:330: controller.endpointsMap.should.eql({});
Maybe avoid mixing testing calls by changing this to:

assert.strictEqual(controller.endpointsMap, {});

[frankban, 2013-04-11 11:32:25]

LGTM. Very nice, thank you! Successfully QA'd.

[gary.poster, 2013-04-11 12:38:52]

*** Submitted:

Fix bug 1166222

The Go environment is unwilling to load a charm before it is part of the
environment, which broke our new endpoints implementation.  Luckily, fixing it
ended up being a nice cleanup on our side, I think.  We do not need the charm
until it is no longer a ghost.

R=teknico, frankban
CC=
https://codereview.appspot.com/8647043

https://codereview.appspot.com/8647043/diff/1/app/store/endpoints.js
File app/store/endpoints.js (right):

https://codereview.appspot.com/8647043/diff/1/app/store/endpoints.js#newcode156
app/store/endpoints.js:156: */
On 2013/04/11 09:57:13, teknico wrote:
> Why the different style in the handleServiceEvent doc comment definition and
> this one?


handleServiceEvent was a new method I added, and I followed the style decision
the team at least partly settled on last Friday.  Even though it looks like I
added the serviceAddHandler docstring, that's a diff confusion.  See lines
118-122 of the original version.  I did not migrate any existing docstrings,
despite the inconsistency, because we apparently need to have a second round of
the discussion this Friday because Ben was not fully on board.

https://codereview.appspot.com/8647043/diff/1/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8647043/diff/1/test/test_endpoints.js#newcode330
test/test_endpoints.js:330: controller.endpointsMap.should.eql({});
On 2013/04/11 09:57:13, teknico wrote:
> Maybe avoid mixing testing calls by changing this to:
> 
> assert.strictEqual(controller.endpointsMap, {});

OK, cool, I'll do that (deepEqual, actually) in this test (but not in the suite,
or the file, or the project for this branch... :-) )

[gary.poster, 2013-04-11 15:37:43]

Thank you both.  (Replies in previous email.)

Gary