Track endpoints as state.

Track endpoints as state.

Remove the get_endpoints calls in the environment and the API.  Instead
keep the endpoints map up-to-date when services are added,
deleted, and changed.

https://code.launchpad.net/~bac/juju-gui/get-endpoints/+merge/156661

(do not edit description out of merge proposal)

[bac, 2013-04-02 19:21:47]

Please take a look.

[jeff.pihach, 2013-04-02 19:56:43]

This is looking great Thanks for all this work! It really cleans up a lot of
code.

I'd like to open up a discussion between a few of us to see if this should be
moved into it's own closure and then the rest of the comments are pretty
trivial.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js
File app/models/endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:12: var models = Y.namespace('juju.models');
I know that this is how it was done previously but it feels like these methods
are just hanging off of juju.models with all of the other models when they
should really be within their own closure of utility methods to clean up the
object. This should also make it easier to test. Thoughts?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:15: models.endpoints_map = {};
camelCase?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:169: // var requires =
evt.currentTarget.get('requires');
commented out old code that should be removed?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:255: if (true) { // Avoid lint warning.
Is this required so that the linter doesn't complain about the vars being
re-declared below?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:256: var rel = {};
I'm surprised that the linter didn't complain about these vars being re-declared
every loop. Typically `k`, `rel`, `j` would be defined at the top of the fn with
`result` then re-assigned in every loop.

https://codereview.appspot.com/8275043/diff/2001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/models.js#newcode563
app/models/models.js:563: this.endpoints_map = {};
camelCase?

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation....
app/views/topology/relation.js:563: var endpoints = models.getEndpoints(service,
models.endpoints_map, db);
camelCase endpoints_map

https://codereview.appspot.com/8275043/diff/2001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/test/test_endpoints.js#newco...
test/test_endpoints.js:135: var charm = new models.Charm({id:
'cs:precise/wordpress-2'});
Because this attaches events we should destroy it at the end of every test and
make sure that the events that it attaches are destroyed as well.

[bcsaller, 2013-04-02 20:39:56]

This is looking good but I do ask a structural change below. I hope I explain it
well enough (and the reasons for it). If you have questions let me know.

https://codereview.appspot.com/8275043/diff/2001/app/app.js
File app/app.js (left):

https://codereview.appspot.com/8275043/diff/2001/app/app.js#oldcode478
app/app.js:478: 
Nice that this removed so much code from App :)

https://codereview.appspot.com/8275043/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/app.js#newcode364
app/app.js:364: this);
This is fine for now but When I have this many subscriptions I'd usually wrap it
in a controller object such that 

controller._subscriptions = controller.bind()

and controller.unbind can use the subscription object to remove listeners.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js
File app/models/endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:29: models.getEndpoints = function(svc, ep_map, db) {
This signature doesn't work well anymore, the db may or may not be the one the
service handlers were bound to. By making a controller object we can reference
the db when we bind the handlers and drop this argument making it consistent.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:167: charm.on('load', Y.bind(function(svcName, evt) {
It shouldn't matter, but this seems like a case for charm.once(...) rather than
on() as the charm should be static post load and we don't need to keep the
listener active.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:181: models.serviceAddHandler = function(evt) {
If we had a controller here like EndpointManager or something we could hang the
handlers off that rather than models which would be nicer (In addition to the
bind/unbind stuff mentioned before).

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:254: for (var k in meta) {
Why is this done in native JS rather than with each() or map()? (I think) The
linter complaining here is because in a loop we want to know if the current item
is owned by the object or part of its prototype, the higher level constructs can
safely handle those cases for us.

https://codereview.appspot.com/8275043/diff/2001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (left):

https://codereview.appspot.com/8275043/diff/2001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:427: // },
nice catch, thanks for pulling all these from the envs

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation....
app/views/topology/relation.js:563: var endpoints = models.getEndpoints(service,
models.endpoints_map, db);
as noted before, in causes like this we can't always be sure the db is the one
the listeners were bound to.

[jeff.pihach, 2013-04-02 20:57:05]

LGTM with Ben's changes and those instances in the tests destroyed.

Thanks again, Great work!

[bcsaller, 2013-04-02 21:02:45]

LGTM

as discussed online the changes I suggested will be pushed to another branch
(and I am fine with that).

[bac, 2013-04-02 21:31:09]

Thanks for the reviews and the great suggestions.  Since none of them are show
stoppers I'll address them in a follow-on branch so this blocking work can land,
albeit a bit unclean.

https://codereview.appspot.com/8275043/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/app.js#newcode364
app/app.js:364: this);
Great suggestion Ben.  I'll do it in the follow-on branch.  A card has been
created with my face on it.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js
File app/models/endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:15: models.endpoints_map = {};
On 2013/04/02 19:56:43, jeff.pihach wrote:
> camelCase?

Done.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newc...
app/models/endpoints.js:254: for (var k in meta) {
No good reason.  I'll clean that up tomorrow.

https://codereview.appspot.com/8275043/diff/2001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/models.js#newcode563
app/models/models.js:563: this.endpoints_map = {};
On 2013/04/02 19:56:43, jeff.pihach wrote:
> camelCase?

Done.

https://codereview.appspot.com/8275043/diff/2001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/test/test_endpoints.js#newco...
test/test_endpoints.js:135: var charm = new models.Charm({id:
'cs:precise/wordpress-2'});
On 2013/04/02 19:56:43, jeff.pihach wrote:
> Because this attaches events we should destroy it at the end of every test and
> make sure that the events that it attaches are destroyed as well.

Done.

[bac, 2013-04-02 21:38:20]

*** Submitted:

Track endpoints as state.

Remove the get_endpoints calls in the environment and the API.  Instead
keep the endpoints map up-to-date when services are added,
deleted, and changed.

R=jeff.pihach, bcsaller
CC=
https://codereview.appspot.com/8275043