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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

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#newcode907
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.

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

« Back to merge proposal