Find possible relation endpoints for service

Find possible relation endpoints for service

This is the refactoring Kapil and I did of the utility to find
possible endpoints for a service.

https://code.launchpad.net/~bcsaller/juju-ui/iter-get-endpoints/+merge/129299

(do not edit description out of merge proposal)

[bcsaller, 2012-10-11 20:31:14]

Please take a look.

[hazmat, 2012-10-11 22:57:14]

I'm going to refrain from approving this on the basis that i worked on this
branch as well, some minors below though. Re mp text, I'd consider it more a
rewrite then a refactoring.

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

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode8
app/models/endpoints.js:8: - the relationlist.has_relations_for_endpoint check
when dealing
somebody (/me points to himself) forgot his capitalization in this block.

https://codereview.appspot.com/6658046/diff/1/test/sample_endpoints.json
File test/sample_endpoints.json (right):

https://codereview.appspot.com/6658046/diff/1/test/sample_endpoints.json#newc...
test/sample_endpoints.json:1: {"haproxy": {"requires": [{"interface": "http",
"limit": 1, "optional": false, "name": "reverseproxy", "scope": "global"}],
"provides": [{"interface": "munin-node", "limit": null, "optional": false,
"name": "munin", "scope": "global"},{"interface": "http", "limit": null,
"optional": false, "name": "website", "scope": "global"}]},"mediawiki":
{"requires": [{"interface": "mysql", "limit": 1, "optional": false, "name":
"slave", "scope": "global"},{"interface": "memcache", "limit": 1, "optional":
false, "name": "cache", "scope": "global"},{"interface": "mysql", "limit": 1,
"optional": false, "name": "db", "scope": "global"}], "provides": [{"interface":
"http", "limit": null, "optional": false, "name": "website", "scope":
"global"}]},"puppet": {"requires": [{"interface": "puppet", "limit": 1,
"optional": false, "name": "puppetmaster", "scope": "global"},{"interface":
"juju-info", "limit": 1, "optional": false, "name": "juju-info", "scope":
"container"}], "provides": []},"mysql": {"requires": [{"interface":
"mysql-oneway-replication", "limit": 1, "optional": false, "name": "slave",
"scope": "global"}], "provides": [{"interface": "munin-node", "limit": null,
"optional": false, "name": "munin", "scope": "global"},{"interface": "mysql",
"limit": null, "optional": false, "name": "db", "scope": "global"},{"interface":
"mysql-oneway-replication", "limit": null, "optional": false, "name": "master",
"scope": "global"},{"interface": "mysql-shared", "limit": null, "optional":
false, "name": "shared-db", "scope": "global"},{"interface": "local-monitors",
"limit": null, "optional": false, "name": "local-monitors", "scope":
"container"},{"interface": "mysql-root", "limit": null, "optional": false,
"name": "db-admin", "scope": "global"},{"interface": "monitors", "limit": null,
"optional": false, "name": "monitors", "scope": "global"}]},"wordpress":
{"requires": [{"interface": "mount", "limit": 1, "optional": false, "name":
"nfs", "scope": "global"},{"interface": "memcache", "limit": 1, "optional":
false, "name": "cache", "scope": "global"},{"interface": "mysql", "limit": 1,
"optional": false, "name": "db", "scope": "global"}], "provides": [{"interface":
"http", "limit": null, "optional": false, "name": "website", "scope":
"global"}]},"memcached": {"requires": [], "provides": [{"interface":
"munin-node", "limit": null, "optional": false, "name": "munin", "scope":
"global"},{"interface": "memcache", "limit": null, "optional": false, "name":
"cache", "scope": "global"}]}}
now that these are there external, these can be pretty-printed for readability.

https://codereview.appspot.com/6658046/diff/1/test/sample_env.json
File test/sample_env.json (right):

https://codereview.appspot.com/6658046/diff/1/test/sample_env.json#newcode2
test/sample_env.json:2: ["relation", "add", {"interface": "mysql", "scope":
"global", "endpoints": [["mysql", {"role": "server", "name": "db"}],
["mediawiki", {"role": "client", "name": "db"}]], "id": "relation-0000000002"}],
["relation", "add", {"interface": "memcache", "scope": "global", "endpoints":
[["memcached", {"role": "server", "name": "cache"}], ["mediawiki", {"role":
"client", "name": "cache"}]], "id": "relation-0000000003"}], ["relation", "add",
{"interface": "mysql", "scope": "global", "endpoints": [["wordpress", {"role":
"client", "name": "db"}], ["mysql", {"role": "server", "name": "db"}]], "id":
"relation-0000000001"}], ["relation", "add", {"interface": "reversenginx",
"scope": "global", "endpoints": [["wordpress", {"role": "peer", "name":
"loadbalancer"}]], "id": "relation-0000000004"}], ["relation", "add",
{"interface": "juju-info", "scope": "container", "endpoints": [["haproxy",
{"role": "server", "name": "juju-info"}], ["puppet", {"role": "client", "name":
"juju-info"}]], "id": "relation-0000000006"}]]
Now that these are there external, these can be pretty-printed for readability.

[gary.poster, 2012-10-12 01:05:46]

Hi Ben.  This is a nice branch, thank you.  It's approved for landing.  I have a
lot of comments and suggestions, and a few questions.

After this is landed, and after the demo, I'd really like to see a day's worth
of work to refactor this with TDD, now that you better understand the problem. 
The first job would be to note down all the funny edge cases you need to test,
to which some of your TODO comments allude.  Then when you write the
corresponding tests and the code they solve, maybe simpler testing approaches
will emerge.

In particular, I'm wondering if some smaller testable functions will fall out of
this.  As I said inline, the meat of this code is the sort of thing I've written
before too: a big, long, largely procedural essay.  If you want to keep it as an
essay, it needs to be better commented, soon, while you still have the state in
your head.  Maybe better would be to identify ways to break things out and
shorten the main function into something more quickly readable and
understandable.  Well-named, well-factored, and perhaps separately tested helper
functions could make the main function a lot easier to follow.

That's a bar I don't always reach myself.  However, I think a TDD approach could
let you tackle it from a timeboxed perspective, and at the very least get some
good test coverage out of it.

Thanks again,

Gary

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

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode4
app/models/endpoints.js:4: TODO:
Just a suggestion:
I find todo lists in code, either in separate files or at the tops of files, to
be poor replacements for kanban cards (if you intend to work on them soon) or
bugs (if you don't want to forget them for later).  YMMV.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode13
app/models/endpoints.js:13: container rel that need to be explored.
Right now you are deep into the problem.  Later you won't be.  Taking time to
elaborate on your concerns will probably serve your future self, or someone
else, well.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode16
app/models/endpoints.js:16: not being a subordinate.
This *sounds* easy.  If so, maybe this is worth a kanban card (with optional
bug), and the other two are just bugs?

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode22
app/models/endpoints.js:22: models.getEndpoints = function(svc, ep_map, db) {
An explanatory docstring would be really nice.  The goal of the function would
be cool; and what an endpoint map is and looks like would be helpful too.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode31
app/models/endpoints.js:31: function epic(svcName, relInfo) {
"epic" does not convey a meaning to me yet.  A more descriptive name and/or a
descriptive comment might be nice.  I guess this returns some hash about a
relation that is friendly to...something?

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode34
app/models/endpoints.js:34: type: relInfo['interface']};
Within the constraints of our beautifier, I think this whitespace is prettier
(as shown by benji's style guide notes):
return
    { service: svcName,
      name: relInfo.name,
      type: relInfo['interface']};

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode56
app/models/endpoints.js:56: requires.push(ep);
Minor point, and do what you will, but these last four lines seem a bit odd of a
spelling to me.  Aren't you saying "if there are no relations for this endpoint,
add it to 'requires'"?  IOW
if (!db.relations.has_relation_for_endpoint(ep)) {
    requires.push(ep);
}
The "break out early" pattern currently used in the code only makes sense to me
as an exception--if there's a lot more logic coming for the normal case.

Again, do what you think makes sense.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode65
app/models/endpoints.js:65: // Every non subordinate implicitly provides this.
Huh.  That's interesting.  I never would have known.  What's 'juju-info'?

I'd personally be inclined to make the explanation in the code, but if you just
want to tell me sometime I'd appreciate it.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode107
app/models/endpoints.js:107: });
FWIW, this (line 99-107) is another example of when I think the return hinders
comprehension.  I'd say 
if (!db.relations.has_relation_for_endpoint(ep)) {
    Y.Array.each(provides, function(oep) {
        if (oep.type === ep.type) {
            addTarget(sid, ep);
        }
    });
}

YMMV.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode114
app/models/endpoints.js:114: //console.log(" checking provided", ep);
suggest removing.

https://codereview.appspot.com/6658046/diff/1/app/models/endpoints.js#newcode126
app/models/endpoints.js:126: //       more than one subordinate relation.
I suggest a card for reviewing this when we are not in demo mode.  We should
make it high priority next week, while this is still fresh in your mind.

I suggest couching the card as "write comprehensive test suite for endpoint
calculations."  It looks like this is complex enough that this approach is the
right way to go.  Alternatively or in addition, when I write very big functions
like this, I do one of two things.  If I'm tired, or tired of it, or still
experimenting, I copiously comment the logic, so it is clear what each section
is doing.  If I'm fresh, or refactoring, I try to break out the logic into
smaller, very well named and considered bits so that the main function is very
readable.

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

https://codereview.appspot.com/6658046/diff/1/app/models/models.js#newcode420
app/models/models.js:420: return result;
I suspect you already thought of and rejected what I would have done: line 412
would be a "return ..." rather than a "var result = ...".  If you simply hadn't
thought of it, there's the idea.  Do what you will.

https://codereview.appspot.com/6658046/diff/1/app/modules.js
File app/modules.js (right):

https://codereview.appspot.com/6658046/diff/1/app/modules.js#newcode86
app/modules.js:86: requires: ['model', 'model-list', 'juju-endpoints'],
Interesting...so you intend juju-endpoints to simply be a breakout extension of
models that is always loaded, like the Go package model.  I haven't really
embraced that pattern in JS yet.  I'll consider it further.

https://codereview.appspot.com/6658046/diff/1/test/sample_endpoints.json
File test/sample_endpoints.json (right):

https://codereview.appspot.com/6658046/diff/1/test/sample_endpoints.json#newc...
test/sample_endpoints.json:1: {"haproxy": {"requires": [{"interface": "http",
"limit": 1, "optional": false, "name": "reverseproxy", "scope": "global"}],
"provides": [{"interface": "munin-node", "limit": null, "optional": false,
"name": "munin", "scope": "global"},{"interface": "http", "limit": null,
"optional": false, "name": "website", "scope": "global"}]},"mediawiki":
{"requires": [{"interface": "mysql", "limit": 1, "optional": false, "name":
"slave", "scope": "global"},{"interface": "memcache", "limit": 1, "optional":
false, "name": "cache", "scope": "global"},{"interface": "mysql", "limit": 1,
"optional": false, "name": "db", "scope": "global"}], "provides": [{"interface":
"http", "limit": null, "optional": false, "name": "website", "scope":
"global"}]},"puppet": {"requires": [{"interface": "puppet", "limit": 1,
"optional": false, "name": "puppetmaster", "scope": "global"},{"interface":
"juju-info", "limit": 1, "optional": false, "name": "juju-info", "scope":
"container"}], "provides": []},"mysql": {"requires": [{"interface":
"mysql-oneway-replication", "limit": 1, "optional": false, "name": "slave",
"scope": "global"}], "provides": [{"interface": "munin-node", "limit": null,
"optional": false, "name": "munin", "scope": "global"},{"interface": "mysql",
"limit": null, "optional": false, "name": "db", "scope": "global"},{"interface":
"mysql-oneway-replication", "limit": null, "optional": false, "name": "master",
"scope": "global"},{"interface": "mysql-shared", "limit": null, "optional":
false, "name": "shared-db", "scope": "global"},{"interface": "local-monitors",
"limit": null, "optional": false, "name": "local-monitors", "scope":
"container"},{"interface": "mysql-root", "limit": null, "optional": false,
"name": "db-admin", "scope": "global"},{"interface": "monitors", "limit": null,
"optional": false, "name": "monitors", "scope": "global"}]},"wordpress":
{"requires": [{"interface": "mount", "limit": 1, "optional": false, "name":
"nfs", "scope": "global"},{"interface": "memcache", "limit": 1, "optional":
false, "name": "cache", "scope": "global"},{"interface": "mysql", "limit": 1,
"optional": false, "name": "db", "scope": "global"}], "provides": [{"interface":
"http", "limit": null, "optional": false, "name": "website", "scope":
"global"}]},"memcached": {"requires": [], "provides": [{"interface":
"munin-node", "limit": null, "optional": false, "name": "munin", "scope":
"global"},{"interface": "memcache", "limit": null, "optional": false, "name":
"cache", "scope": "global"}]}}
On 2012/10/11 22:57:14, hazmat wrote:
> now that these are there external, these can be pretty-printed for
readability.

+1.  The beautifier won't help though, because it does not insert newlines, so
it's manual.  I personally would be fine with the pretty printing happening in a
later branch.

https://codereview.appspot.com/6658046/diff/1/test/sample_env.json
File test/sample_env.json (right):

https://codereview.appspot.com/6658046/diff/1/test/sample_env.json#newcode2
test/sample_env.json:2: ["relation", "add", {"interface": "mysql", "scope":
"global", "endpoints": [["mysql", {"role": "server", "name": "db"}],
["mediawiki", {"role": "client", "name": "db"}]], "id": "relation-0000000002"}],
["relation", "add", {"interface": "memcache", "scope": "global", "endpoints":
[["memcached", {"role": "server", "name": "cache"}], ["mediawiki", {"role":
"client", "name": "cache"}]], "id": "relation-0000000003"}], ["relation", "add",
{"interface": "mysql", "scope": "global", "endpoints": [["wordpress", {"role":
"client", "name": "db"}], ["mysql", {"role": "server", "name": "db"}]], "id":
"relation-0000000001"}], ["relation", "add", {"interface": "reversenginx",
"scope": "global", "endpoints": [["wordpress", {"role": "peer", "name":
"loadbalancer"}]], "id": "relation-0000000004"}], ["relation", "add",
{"interface": "juju-info", "scope": "container", "endpoints": [["haproxy",
{"role": "server", "name": "juju-info"}], ["puppet", {"role": "client", "name":
"juju-info"}]], "id": "relation-0000000006"}]]
On 2012/10/11 22:57:14, hazmat wrote:
> Now that these are there external, these can be pretty-printed for
readability.

Same reply as before: +1, but later is fine as far as I am concerned.

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

https://codereview.appspot.com/6658046/diff/1/test/test_endpoints.js#newcode71
test/test_endpoints.js:71: sample_env = loadFixture('sample_env.json');
You know, I like the cleanliness here, but upon reading your test, I really
missed being able to see the data source in line, to figure out the world in
which the tests operate.  Maybe beautifying the json will be good enough.

https://codereview.appspot.com/6658046/diff/1/test/test_endpoints.js#newcode93
test/test_endpoints.js:93: done();
done isn't necessary here; might be nice to remove it.

https://codereview.appspot.com/6658046/diff/1/test/test_endpoints.js#newcode96
test/test_endpoints.js:96: it('should be able find relatable services',
function() {
typo: ...able to find...

https://codereview.appspot.com/6658046/diff/1/test/test_endpoints.js#newcode99
test/test_endpoints.js:99: service, default_endpoints, db));
I know this is my knowledge deficiency, but I still don't know where these
endpoints would *really* come from, in the app.

https://codereview.appspot.com/6658046/diff/1/test/test_endpoints.js#newcode132
test/test_endpoints.js:132: it('should be able find ignore existing relations
services', function() {
"should be able to ignore already related services"? Can't quite guess the
correct parsing of that.

https://codereview.appspot.com/6658046/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/6658046/diff/1/test/test_model.js#newcode307
test/test_model.js:307: it('RelationList.has_relations_for_endpoint should do
what it says',
heh

[hazmat, 2012-10-12 01:57:31]

On 2012/10/12 01:05:46, gary.poster wrote:
> Hi Ben.  This is a nice branch, thank you.  It's approved for landing.  I have
a
> lot of comments and suggestions, and a few questions.


Gary, thanks for the detailed review! I'll be tackling these and the internal
todo items, as you said its much better to tackle/finish the deep voodoo when
its fresh in the head.