Implemented add-relation for the fakebackend

Implemented add-relation for the fakebackend

In order to implement add-relation for the fakebackend
a large change was needed in the fake charmstore in 
tests/utils.js in order to allow us to load in real
charm data.

Add-relation also appears to be the only command which
has it's input data altered in the python.js endpoint
so it needs to support two different types of input
while maintaining as close to identical responses.

https://code.launchpad.net/~hatch/juju-gui/add-relation/+merge/157915

(do not edit description out of merge proposal)

[jeff.pihach, 2013-04-09 16:46:32]

Please take a look.

[bcsaller, 2013-04-09 17:40:26]

Thanks for the work on this. There are some trivial notes but given the
complexity of the relationship code I need to see testing of the fakebackend
code as outlined. 

Let me know if you have additional questions.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:523: if (endpointB === undefined) {
add isAuth check

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:523: if (endpointB === undefined) {
This check would still be endpointA === undefined or endpointB === undefined,
no, might as well catch passing bad data.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:573: Y.Array.each([charmA, charmB], addJujuInfo);
I think this needs to be done at the point of consideration (may A relate to B),
not stored on the charm.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:577: 
You need test in test_fakebackend. We should be directly asserting these code
paths. At minimum we'd want the following.

// Inferred interface
addRelation("wordpress", "mysql")
addRelation("mysql", "wordpress")

// Explicit Interface
addRelation("mysql:db", "wordpress:db")
addRelation("wordpress:db", "mysql:db")

// Subordinates
addRelation("wordpress", "puppet")
addRelation("puppet", "wordpress")

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:601: if (ci) { return {ci: ci, cs: cs}; }
I'd prefer clearer names here.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:642: return false;
I expect this will need to return {error: {String} || result: {Object}} once you
add checks like isAuth

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:439: (endpoint === undefined)) { return endpoint; }
trivial but I think you want newlines in the {}.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:444: var res = this.get('state').addRelation(
I'd rename the var to relation if you're going to do things like take
.displayName later. It will be more clear that this isn't an {Object} with
result/error.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:447: var endpointA, endpointB;
These are unused.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:459: // shouldn't be an issue.
I'm fine with this for now, we might have to change it in the future, but you
indicated this.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:461: eptB[data.endpoint_b.split(':')[0]] = {
If 'a' is a string then 'b' is, I guess that's likely true.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:477: data.err = 'Unable to create relation';
This can be moved above the other checks to right after the assignment, no?
Additionally there will be a authenticated check which could error out.

https://codereview.appspot.com/8578043/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/8578043/diff/1/test/utils.js#newcode85
test/utils.js:85: };
test_fakebackend directly overrides loadByPath:177ish. Won't patching the
prototype here bleed into other tests? I don't think this is the way to do this.
Additionally this method still has console.logs in it.

[gary.poster, 2013-04-09 18:16:10]

Jeff, this is great work.  Thank you.

Like Ben, I have a lot of comments, but please don't let it discourage you. 
Great job.

Gary

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:522: addRelation: function(endpointA, endpointB) {
Please add the UNAUTHORIZED check thing that is in all the other methods

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:523: if (endpointB === undefined) {
how about 

if (!endpointA || !endpointB) {...

 - might as well check both, yeah?
 - any falsey value won't work, yeah?

If you want to be really precise, you could say

if (!Y.Lang.isString(endpointA) || !Y.Lang.isString(endpointB) {
  return {error: 'Two string endpoint names required to establish a relation'};
}

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:547: return {error: 'Endpoints must be
different.'};
I have a vague suspicion that it's theoretically possible to have an endpoint
relate with itself.  Please double check with Ben.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:557: });
hm.  I'm concerned about this.  We actually need the charm for the services that
we are going to relate--that is, the specific version.

I would suggest instead finding the services to which we are referring, getting
their exact charm ids, and looking them up directly.  Maybe doublecheck with Ben
that this is sane?

Moreover, the epAData.name and epBData.name values are service names, not charm
names.  they are charm names by default, but not necessarily.  So you definitely
need to go to the service first, and then find the charm from that.

Doublecheck with Ben and/or talk it over with me.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:559: if (!charmsFound) { return {error: 'Charm not
loaded.'}; }
Shouldn't be necessary since we can only deploy if we have a charm, but nice to
check.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:587: function getInterfaceAndScope(charmEndpoints,
charmDatas) {
addRelation is such a big function that it might be nice to divide it up into
composite, separately tested functions.  this kind of thing that is already a
separate function would be one approach, though I like the fact that it is
readable inline.  Think about how you'd like to read this function later, and
divide it up.

TBH and IMO, this is precisely the kind of code that should be prototyped, moved
aside and discarded, and rewritten with TDD.  The rewrite and TDD IME encourages
nicely readable code.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:612: // successfull relation.
typo: successful

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:444: var res = this.get('state').addRelation(
On 2013/04/09 17:40:26, bcsaller wrote:
> I'd rename the var to relation if you're going to do things like take
> .displayName later. It will be more clear that this isn't an {Object} with
> result/error.

+1

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:454: if (typeof data.endpoint_a === 'string') {
This translation code could be factored out and tested separately.  It would
make the main function read more nicely IMO, and it would make for simpler
thorough tests.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:478: this.get('client').receiveNow(data);
This should be receive, not receiveNow, right?  There's nothing async up to this
point AFAICT.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:484: this.get('client').receiveNow(data);
This should be receive, not receiveNow, right?  There's nothing async up to this
point AFAICT.

https://codereview.appspot.com/8578043/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/8578043/diff/1/test/utils.js#newcode67
test/utils.js:67: Y.juju.CharmStore.prototype.loadByPath = function(path,
options) {
Nice!  Awesome, even!  As I said I'd like this to be a subclass instead, though,
please.

[gary.poster, 2013-04-09 18:17:19]

BTW, please add a test in which you are making a relation between two services
that are given names other than their charm names (like kumquat, in other
examples).  I think that will expose a bug.

[jeff.pihach, 2013-04-09 22:15:36]

Thanks for the reviews - only two things still under development. Hoping to get
it re-proposed tomorrow morning

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:522: addRelation: function(endpointA, endpointB) {
On 2013/04/09 18:16:10, gary.poster wrote:
> Please add the UNAUTHORIZED check thing that is in all the other methods

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:523: if (endpointB === undefined) {
On 2013/04/09 17:40:26, bcsaller wrote:
> add isAuth check

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:523: if (endpointB === undefined) {
On 2013/04/09 18:16:10, gary.poster wrote:
> how about 
> 
> if (!endpointA || !endpointB) {...
> 
>  - might as well check both, yeah?
>  - any falsey value won't work, yeah?
> 
> If you want to be really precise, you could say
> 
> if (!Y.Lang.isString(endpointA) || !Y.Lang.isString(endpointB) {
>   return {error: 'Two string endpoint names required to establish a
relation'};
> }

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:547: return {error: 'Endpoints must be
different.'};
removed

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:557: });
Good find, definitely caused a bug that would not have been easily to debug.

Done

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:559: if (!charmsFound) { return {error: 'Charm not
loaded.'}; }
Added comment along those lines.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:573: Y.Array.each([charmA, charmB], addJujuInfo);
developing...

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:577: 
developing...

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:587: function getInterfaceAndScope(charmEndpoints,
charmDatas) {
Split this fn out of the add-relation fn

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:601: if (ci) { return {ci: ci, cs: cs}; }
On 2013/04/09 17:40:26, bcsaller wrote:
> I'd prefer clearer names here. 

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:612: // successfull relation.
On 2013/04/09 18:16:10, gary.poster wrote:
> typo: successful

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:642: return false;
The only time it should ever get here is if the call to
this.db.relations.create() fails which I'm not sure is possible without the
whole application crashing down so it's here more-so as a catch-all JIC

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:439: (endpoint === undefined)) { return endpoint; }
On 2013/04/09 17:40:26, bcsaller wrote:
> trivial but I think you want newlines in the {}.

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:444: var res = this.get('state').addRelation(
On 2013/04/09 17:40:26, bcsaller wrote:
> I'd rename the var to relation if you're going to do things like take
> .displayName later. It will be more clear that this isn't an {Object} with
> result/error.

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:447: var endpointA, endpointB;
On 2013/04/09 17:40:26, bcsaller wrote:
> These are unused.

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:454: if (typeof data.endpoint_a === 'string') {
On 2013/04/09 18:16:10, gary.poster wrote:
> This translation code could be factored out and tested separately.  It would
> make the main function read more nicely IMO, and it would make for simpler
> thorough tests.

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:477: data.err = 'Unable to create relation';
It has to be after this normalization because the error object needs to have the
formatted  endpoint data.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:478: this.get('client').receiveNow(data);
On 2013/04/09 18:16:10, gary.poster wrote:
> This should be receive, not receiveNow, right?  There's nothing async up to
this
> point AFAICT.

Done.

https://codereview.appspot.com/8578043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:484: this.get('client').receiveNow(data);
On 2013/04/09 18:16:10, gary.poster wrote:
> This should be receive, not receiveNow, right?  There's nothing async up to
this
> point AFAICT.

Done.

https://codereview.appspot.com/8578043/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/8578043/diff/1/test/utils.js#newcode67
test/utils.js:67: Y.juju.CharmStore.prototype.loadByPath = function(path,
options) {
On 2013/04/09 18:16:10, gary.poster wrote:
> Nice!  Awesome, even!  As I said I'd like this to be a subclass instead,
though,
> please.

Done.

https://codereview.appspot.com/8578043/diff/1/test/utils.js#newcode85
test/utils.js:85: };
It definitely will effect other tests. This is what caused the deploy test to
fail that I mentioned in the call but it shouldn't cause any issues as long as
the test requests available charm data.

[jeff.pihach, 2013-04-09 23:20:15]

Please take a look.

[gary.poster, 2013-04-10 14:56:35]

Looking great!  some cleanup suggestions below.  To reiterate what I tried to
say inline:

- module functions vs methods is up to you.  Just saying what I would do, for
consideration.

- Pair arrays vs. individual A/B variables is also up to you. Though I do think
the pair arrays are a better, cleaner way that reduces duplication, I'm willing
to concede that it might be stylistic :-).

- Using getById vs. iterating: please use getById! :-)

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:429: endpointSplit: function(endpoint) {
module-level (and exposed) function rather than method? that's what I would do.
up to you.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:444: getCharmInterfaceAndScope:
function(charmEndpoints, charmDatas) {
function rather than method? that's what I would do. up to you.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:577: charmA, charmB, charmId, counter = 0;
fwiw, when I have pairs that I am doing a lot of the same things to, I like to
use arrays, even though they are only two.  I think it looks cleaner.  YMMV, but
I'll give examples as I go.

var endpoints = Y.Array.filter([endpointA, endpointB], this.endpointSplit);

(You might need a bind in there, but it doesn't look like it.)

Future suggestions below would allow you to eliminate charmA, charmB, and
counter from this block as well.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:579: if (epAData.type !== epBData.type) {
if (endpoints[0].type !== endpoints[1].type) {...

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:608: if (!charmsFound) { return {error: 'Charm not
loaded.'}; }
Service name is the same as service id, and is the modellist key; a service's
charm value is the modellist key.  Therefore you can simplify the above two
blocks (lines 583-608) to the following.  I'm using my preferred array approach;
translate it if you don't like it.  This is also an untested sketch.

var self = this;
var charms = Y.Array.filter(endpointData, function(endpoint) {
  var service = self.db.services.getById(endpoint.name);
  if service {
    return self.db.charms.getById(service.get('charm'));
  }
});
// This error should never be hit but it's here JIC
if (!charms[0] || !charms[1]) {
  return {error: 'Charm not loaded.'};
}

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:624: Y.Array.each([charmA, charmB], addJujuInfo);
Hey look, this could be "Y.array.each(charms, addJujuInfo);" ;-)

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:627: charmBEp =
charmB.get('requires')[epBData.type];
Maybe?

var charmEndpoints = Y.Array.filter(Y.Array.zip(charms, endpoints),
function(data) {
  var charm = data[0], endpoint = data[1];
  return charm.get('requires')[endpoint.type];
});

Copying code might actually be better in this case.

var charmEndpoints = [charms[0].get('requires')[endpoints[0].type],
charms[1].get('requires')[endpoints[1].type]];

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:307: endpointToName: function(endpoint) {
function? up to you.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:330: normalizeEndpointFormat: function(endpointA,
endpointB, relation) {
function? up to you.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:499: return;
That function is much nicer to read in isolation, thank you.

[gary.poster, 2013-04-10 14:58:55]

LGTM to land with immediate follow-ons for other changes as discussed.

[jeff.pihach, 2013-04-10 15:48:23]

Thanks for the follow-up review, changes landing shortly.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:429: endpointSplit: function(endpoint) {
On 2013/04/10 14:56:36, gary.poster wrote:
> module-level (and exposed) function rather than method? that's what I would
do.
> up to you.

Done.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:444: getCharmInterfaceAndScope:
function(charmEndpoints, charmDatas) {
On 2013/04/10 14:56:36, gary.poster wrote:
> function rather than method? that's what I would do. up to you.

Done.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:577: charmA, charmB, charmId, counter = 0;
done but used map() instead.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:579: if (epAData.type !== epBData.type) {
On 2013/04/10 14:56:36, gary.poster wrote:
> if (endpoints[0].type !== endpoints[1].type) {...

Done.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:608: if (!charmsFound) { return {error: 'Charm not
loaded.'}; }
done with map()

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:624: Y.Array.each([charmA, charmB], addJujuInfo);
On 2013/04/10 14:56:36, gary.poster wrote:
> Hey look, this could be "Y.array.each(charms, addJujuInfo);" ;-)

Done.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:627: charmBEp =
charmB.get('requires')[epBData.type];
On 2013/04/10 14:56:36, gary.poster wrote:
> Maybe?
> 
> var charmEndpoints = Y.Array.filter(Y.Array.zip(charms, endpoints),
> function(data) {
>   var charm = data[0], endpoint = data[1];
>   return charm.get('requires')[endpoint.type];
> });
> 
> Copying code might actually be better in this case.
> 
> var charmEndpoints = [charms[0].get('requires')[endpoints[0].type],
> charms[1].get('requires')[endpoints[1].type]];

Done.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:307: endpointToName: function(endpoint) {
On 2013/04/10 14:56:36, gary.poster wrote:
> function? up to you.

Done.

https://codereview.appspot.com/8578043/diff/10001/app/store/env/sandbox.js#ne...
app/store/env/sandbox.js:330: normalizeEndpointFormat: function(endpointA,
endpointB, relation) {
On 2013/04/10 14:56:36, gary.poster wrote:
> function? up to you.

Done.

[jeff.pihach, 2013-04-10 15:58:04]

Please take a look.

[bcsaller, 2013-04-10 16:10:06]

On 2013/04/10 15:58:04, jeff.pihach wrote:
> Please take a look.

LGTM with the plan you have for moving forward. Other stuff can land on another
branch, thanks!

[jeff.pihach, 2013-04-10 16:52:34]

*** Submitted:

Implemented add-relation for the fakebackend

In order to implement add-relation for the fakebackend
a large change was needed in the fake charmstore in 
tests/utils.js in order to allow us to load in real
charm data.

Add-relation also appears to be the only command which
has it's input data altered in the python.js endpoint
so it needs to support two different types of input
while maintaining as close to identical responses.

R=bcsaller, gary.poster
CC=
https://codereview.appspot.com/8578043