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

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

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#newcode610
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#newcode780
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#newcode842
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.

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

« Back to merge proposal