status: added the display of subordinates

status: added the display of subordinates

Added the display of subordinated units below their
associated units and "subordinate-to" for the
subordinated services.

https://code.launchpad.net/~themue/juju-core/020-cmd-status-subordinates/+merge/159339

(do not edit description out of merge proposal)

[TheMue, 2013-04-17 10:10:45]

Please take a look.

[fwereade, 2013-04-17 11:12:12]

Excellent start, but a couple of tweaks needed. Only the ones that actually
affect output are showstoppers though.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status.go#oldcode183
cmd/juju/status.go:183: }
This all feels more complex than it needs to be, but if it can support the
changes requested in the other file I'm not too bothered today.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status.go#newcode271
cmd/juju/status.go:271: if unit.IsPrincipal() {
Not required.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode570
cmd/juju/status_test.go:570: setUnitStatus{"logging/1", params.StatusStarted,
""},
Maybe just have one of these with a non-"" status?

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode622
cmd/juju/status_test.go:622: "units": M{
logging should not show its own units.

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode845
cmd/juju/status_test.go:845: func (rsws relateServicesWithScope) step(c *C, ctx
*context) {
I think the relateServices should be separate, and there should be a separate

type addSubordinate struct {
    principal string
    subService string
}

...that does roughly the same, but using the specific correct unit rather than
just picking the same one every time.

[TheMue, 2013-04-17 13:06:31]

Please take a look.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status.go#oldcode183
cmd/juju/status.go:183: }
On 2013/04/17 11:12:12, fwereade wrote:
> This all feels more complex than it needs to be, but if it can support the
> changes requested in the other file I'm not too bothered today.

Done some renamings that hopefully make it clearer. The usage of two maps, one
keyed by those units which have subordinates, the other by services that are
subordinates, makes it simple to avoid nesting or iterating over all services
and their units to look if one those has a subordinate (would also need a
different data structure in the 1st pass).

A statusMap.get(keys ...string) interface{} could also help improve overall
reading.

Other good ideas are welcome.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status.go#newcode271
cmd/juju/status.go:271: if unit.IsPrincipal() {
On 2013/04/17 11:12:12, fwereade wrote:
> Not required.

Done.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode570
cmd/juju/status_test.go:570: setUnitStatus{"logging/1", params.StatusStarted,
""},
On 2013/04/17 11:12:12, fwereade wrote:
> Maybe just have one of these with a non-"" status?

Done.

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode622
cmd/juju/status_test.go:622: "units": M{
On 2013/04/17 11:12:12, fwereade wrote:
> logging should not show its own units.

Done.

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode845
cmd/juju/status_test.go:845: func (rsws relateServicesWithScope) step(c *C, ctx
*context) {
On 2013/04/17 11:12:12, fwereade wrote:
> I think the relateServices should be separate, and there should be a separate
> 
> type addSubordinate struct {
>     principal string
>     subService string
> }
> 
> ...that does roughly the same, but using the specific correct unit rather than
> just picking the same one every time.

Done.

[fwereade, 2013-04-17 13:38:28]

LGTM wrt output. Code improvements appreciated but not required.

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

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status.go#newcode214
cmd/juju/status.go:214: delete(servicesMap[serviceName].(statusMap), "units")
I remain suspicious of this, but the output looks sane, we can worry about this
another day.

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status.go#newcode283
cmd/juju/status.go:283: subTo = subToMap[svcName].([]string)
Why are we doing this over and over again for every unit?

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

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:610: "agent-state-info": "somehow lost in all those
logs",
:)

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:780: s, err := ctx.st.Service(sua.serviceName)
Do we have anything for testing non-agent-alive units?

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:842: rel, err := ctx.st.AddRelation(eps...)
Please don't do this. Create the relation separately, and get it here with
Relation.

[TheMue, 2013-04-17 15:25:46]

Please take a look.

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

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status.go#newcode214
cmd/juju/status.go:214: delete(servicesMap[serviceName].(statusMap), "units")
On 2013/04/17 13:38:28, fwereade wrote:
> I remain suspicious of this, but the output looks sane, we can worry about
this
> another day.

Will add some helpful comments that hopefully show how simple this is by
principle.

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status.go#newcode283
cmd/juju/status.go:283: subTo = subToMap[svcName].([]string)
On 2013/04/17 13:38:28, fwereade wrote:
> Why are we doing this over and over again for every unit?

Only for those units with subordinates. Ensure a string slice to append the
current units name to. Will add comments and change names.

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

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:780: s, err := ctx.st.Service(sua.serviceName)
On 2013/04/17 13:38:28, fwereade wrote:
> Do we have anything for testing non-agent-alive units?

Look for expect with "add two more machine, one with a dead agent, one in error
state". Does this cover your question or do you have a different thing in mind?

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newc...
cmd/juju/status_test.go:842: rel, err := ctx.st.AddRelation(eps...)
On 2013/04/17 13:38:28, fwereade wrote:
> Please don't do this. Create the relation separately, and get it here with
> Relation.

Done.

[dimitern, 2013-04-17 15:32:45]

Somewhat grudgy LGTM, although I don't like how the tests and readability are
both deteriorating. The output is fine, that's what's important and both issues
above can be fixed in a follow up.

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

https://codereview.appspot.com/8824043/diff/12001/cmd/juju/status.go#newcode182
cmd/juju/status.go:182: // prinToSubUnitMap will collect mappings from principal
unit names
readability here really could be improved, but i'm not blocking this for that.

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

https://codereview.appspot.com/8824043/diff/12001/cmd/juju/status_test.go#new...
cmd/juju/status_test.go:907: 
this is *DEFINITELY WRONG*, yet another copy of exactly the same test driver
function as TestStatusAllFormats and TestRelations. I'll fix this in a follow up
though, not blocking.

[fwereade, 2013-04-17 22:05:46]

Frank, sorry: rogpeppe got carried away and proposed a branch that cleans up the
structure so much it's worth the duplicated effort IMO... so NOT LGTM. Your
tests live on, though, and are much appreciated.