status: added display of relations for services

status: added display of relations for services

Implemented the display of relations between services.

https://code.launchpad.net/~themue/juju-core/019-cmd-status-relations-services/+merge/158570

(do not edit description out of merge proposal)

[TheMue, 2013-04-12 11:48:04]

Please take a look.

[fwereade, 2013-04-12 14:40:47]

Where did you get the idea that relation status looks like this?

https://codereview.appspot.com/8705043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/1/cmd/juju/status.go#newcode246
cmd/juju/status.go:246: relations, err := service.Relations()
It's be nice to get all relations just once, instead of getting most of them
twice over N roundtrips. Probably needs a state change; if so, please just TODO
it.

https://codereview.appspot.com/8705043/diff/1/cmd/juju/status.go#newcode256
cmd/juju/status.go:256: sm[relation.String()] = endpoint.String()
This bears no relation to the python output that I can detect.

[TheMue, 2013-04-13 16:02:33]

Please take a look.

https://codereview.appspot.com/8705043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/1/cmd/juju/status.go#newcode246
cmd/juju/status.go:246: relations, err := service.Relations()
On 2013/04/12 14:40:47, fwereade wrote:
> It's be nice to get all relations just once, instead of getting most of them
> twice over N roundtrips. Probably needs a state change; if so, please just
TODO
> it.

Done.

https://codereview.appspot.com/8705043/diff/1/cmd/juju/status.go#newcode256
cmd/juju/status.go:256: sm[relation.String()] = endpoint.String()
On 2013/04/12 14:40:47, fwereade wrote:
> This bears no relation to the python output that I can detect.

Absolutely, sorry. Have been misled by the first approach and by understanding
the Python code (last active Py development has been 10 years ago). Especially I
mixed up the two maps/dict maintained in the two loops (still not sure where the
relations are iterated twice). 

I've now tried to change it to work more similar to the Python implementation.
Especially with respect to the usage of the relation service map later when
processing the units.

[fwereade, 2013-04-15 00:36:30]

This still doesn't match, as far as I'm aware.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode281
cmd/juju/status.go:281: return nil, nil, nil, fmt.Errorf("unexpected
relationship with more than 2 endpoints")
s/ship//

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode283
cmd/juju/status.go:283: relMap.addToSet(strconv.Itoa(relation.Id()),
eps[0].ServiceName)
This is definitely wrong. We should not be keying on relation id at all here,
let alone on a string representation thereof.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode306
cmd/juju/status.go:306: return nil, fmt.Errorf("unexpected relationship with
more than 2 endpoints")
s/ship//

Also, this large loop seems to be duplicated across pR and pRM, the purpose of
which eludes me. It feels like all this should just be:

relMap := make(statusMap)
for _, rel := range relations {
    ep, err := relation.Endpoint(service.Name())
    if err != nil {
        return err
    }
    relName := ep.Relation.Name
    eps, err := relation.RelatedEndpoints(service.Name())
    if err != nil {
        return err
    }
    serviceNames := []string{}
    for _, ep := range eps {
        serviceNames = append(serviceNames, ep.ServiceName)
    }
    sort.Strings(serviceNames)
    relMap[relName] = serviceNames
}

...and that it only needs to be done once. Am I missing something?

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode330
cmd/juju/status.go:330: func (sm statusMap) addToSet(key, value string) {
Given the above, I question the utility of these new methods.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode348
cmd/juju/status.go:348: func (sm statusMap) processVersion(v versioned) {
This is not used. If it were used, the other processVersion should be dropped,
and processStatus and checkError should become methods on statusMap... but I'd
prefer that we not get sidetracked by that here.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:397: addService{"a-service", "dummy"},
"mysql", "mysql"

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:404: addService{"b-service", "dummy"},
"wordpress", "wordpress"

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:411: addService{"c-service", "dummy"},
"another-wordpress", "wordpress"

...or whatever. But, please, something more meaningful than a, b, c :).

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:441: "0": L{"b-service"},
The key should be the relation name from the point of view of the service it's
under. Something like:

services:
  wordpress:
    relations:
      db: [mysql]
  another-wordpress:
    relations:
      db: [mysql]
  mysql: 
    relations:
      server: [wordpress,another-wordpress]

Please consider context... if the key really were relation id, there'd be no
sense at all in the value being a list.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:538: requires map[string]charm.Relation
Please just use the standard testing charms (wordpress, mysql, riak, logging)
which offer, I think, all distinct relation types amongst themselves; and stick
with "name" as the only field here.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:678: }
type relateServices struct{
    ep1, ep2 string
}

func (rs relateServices) step(c *C, ctx *context) {
    eps, err := ctx.st.InferEndpoints([]string{rs.ep1, rs.ep2})
    c.Assert(err, IsNil)
    _, err := ctx.st.AddRelation(eps...)
    c.Assert(err, IsNil)
}

...or something like that anyway.

[TheMue, 2013-04-15 12:20:41]

Please take a look.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode281
cmd/juju/status.go:281: return nil, nil, nil, fmt.Errorf("unexpected
relationship with more than 2 endpoints")
On 2013/04/15 00:36:30, fwereade wrote:
> s/ship//

Came from the Python code, but now removed due to your approach.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode283
cmd/juju/status.go:283: relMap.addToSet(strconv.Itoa(relation.Id()),
eps[0].ServiceName)
On 2013/04/15 00:36:30, fwereade wrote:
> This is definitely wrong. We should not be keying on relation id at all here,
> let alone on a string representation thereof.

Sorry, has been my interpretation of

relation_data.setdefault(relation.relation_name,
set()).add(rel_service.service_name)

in the Python code.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode306
cmd/juju/status.go:306: return nil, fmt.Errorf("unexpected relationship with
more than 2 endpoints")
On 2013/04/15 00:36:30, fwereade wrote:
> s/ship//
> 
> Also, this large loop seems to be duplicated across pR and pRM, the purpose of
> which eludes me. It feels like all this should just be:
> 
> relMap := make(statusMap)
> for _, rel := range relations {
>     ep, err := relation.Endpoint(service.Name())
>     if err != nil {
>         return err
>     }
>     relName := ep.Relation.Name
>     eps, err := relation.RelatedEndpoints(service.Name())
>     if err != nil {
>         return err
>     }
>     serviceNames := []string{}
>     for _, ep := range eps {
>         serviceNames = append(serviceNames, ep.ServiceName)
>     }
>     sort.Strings(serviceNames)
>     relMap[relName] = serviceNames
> }
> 
> ...and that it only needs to be done once. Am I missing something?

IMHO not, I already wondered myself. This is a copy of the Python approach and I
hoped to get aware of why the relations are processed twice when implementing
the relation units.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode330
cmd/juju/status.go:330: func (sm statusMap) addToSet(key, value string) {
On 2013/04/15 00:36:30, fwereade wrote:
> Given the above, I question the utility of these new methods.

Convenience, cleaned up after taking your approach and the new sting set.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status.go#newcode348
cmd/juju/status.go:348: func (sm statusMap) processVersion(v versioned) {
On 2013/04/15 00:36:30, fwereade wrote:
> This is not used. If it were used, the other processVersion should be dropped,
> and processStatus and checkError should become methods on statusMap... but I'd
> prefer that we not get sidetracked by that here.

Aaaargh, yes, missed to change the usage of the already existing function. But
now with the simpler approach I changed it back and removed this method.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:397: addService{"a-service", "dummy"},
On 2013/04/15 00:36:30, fwereade wrote:
> "mysql", "mysql"

Done.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:404: addService{"b-service", "dummy"},
On 2013/04/15 00:36:30, fwereade wrote:
> "wordpress", "wordpress"

Done.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:411: addService{"c-service", "dummy"},
On 2013/04/15 00:36:30, fwereade wrote:
> "another-wordpress", "wordpress"
> 
> ...or whatever. But, please, something more meaningful than a, b, c :).

Done. But also "dummy" isn't very meaningful. ;)

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:538: requires map[string]charm.Relation
On 2013/04/15 00:36:30, fwereade wrote:
> Please just use the standard testing charms (wordpress, mysql, riak, logging)
> which offer, I think, all distinct relation types amongst themselves; and
stick
> with "name" as the only field here.

Done.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:538: requires map[string]charm.Relation
On 2013/04/15 00:36:30, fwereade wrote:
> Please just use the standard testing charms (wordpress, mysql, riak, logging)
> which offer, I think, all distinct relation types amongst themselves; and
stick
> with "name" as the only field here.

Done.

https://codereview.appspot.com/8705043/diff/5001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:678: }
On 2013/04/15 00:36:30, fwereade wrote:
> type relateServices struct{
>     ep1, ep2 string
> }
> 
> func (rs relateServices) step(c *C, ctx *context) {
>     eps, err := ctx.st.InferEndpoints([]string{rs.ep1, rs.ep2})
>     c.Assert(err, IsNil)
>     _, err := ctx.st.AddRelation(eps...)
>     c.Assert(err, IsNil)
> }
> 
> ...or something like that anyway.

Done.

[fwereade, 2013-04-15 14:51:49]

LGTM, thanks.

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go#newcode285
cmd/juju/status.go:285: relationMap[relationName] = sn.SortedValues()
Don't quite understand why this is necessary, but I guess it matches python. Add
a comment?

[dimitern, 2013-04-15 15:12:40]

Almost there - how about peer relations?

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go#newcode256
cmd/juju/status.go:256: // Maybe add Relations() to state, read them only once
and pass them to the to each
s/to the//

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go#newcode300
cmd/juju/status.go:300: type statusMap map[string]interface{}
can you move this at the top please?

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:372: addUnit{"project", "1"},
please use addAliveUnit + setUnitStatus for at least 2 of the units you're
adding, and adjust the expected output accordingly.

[TheMue, 2013-04-16 07:54:07]

Please take a look.

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go#newcode256
cmd/juju/status.go:256: // Maybe add Relations() to state, read them only once
and pass them to the to each
On 2013/04/15 15:12:40, dimitern wrote:
> s/to the//

Done.

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go#newcode285
cmd/juju/status.go:285: relationMap[relationName] = sn.SortedValues()
On 2013/04/15 14:51:49, fwereade wrote:
> Don't quite understand why this is necessary, but I guess it matches python.
Add
> a comment?

Done.

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status.go#newcode300
cmd/juju/status.go:300: type statusMap map[string]interface{}
On 2013/04/15 15:12:40, dimitern wrote:
> can you move this at the top please?

Done.

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8705043/diff/11001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:372: addUnit{"project", "1"},
On 2013/04/15 15:12:40, dimitern wrote:
> please use addAliveUnit + setUnitStatus for at least 2 of the units you're
> adding, and adjust the expected output accordingly.

Done.

[dimitern, 2013-04-16 11:38:50]

Thanks for the peer tests, although I don't think it's worth having a whole
separate table and test case method just for relations. Otherwise, LGTM.

[TheMue, 2013-04-16 12:15:03]

*** Submitted:

status: added display of relations for services

Implemented the display of relations between services.

R=fwereade, dimitern
CC=
https://codereview.appspot.com/8705043