Code review comment for lp:~themue/juju-core/020-cmd-status-subordinates

Revision history for this message
William Reade (fwereade) wrote :

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.

https://codereview.appspot.com/8824043/

« Back to merge proposal