Code review comment for lp:~axwalk/juju-core/lp1121916-juju-status-filters

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

LGTM, thanks -- sorry for the delay, it's a busy week.

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

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode47
cmd/juju/status.go:47: if and when they are tried.`[1:]
Maybe add a little bit more explaining which bits are filtered out (and
which bits are not).

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode52
cmd/juju/status.go:52: Args: "[pattern...]",
s/n./n ./

?

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode116
cmd/juju/status.go:116: } else {
lose the else?

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

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#newcode34
cmd/juju/status_test.go:34: stderr = ctx.Stderr.(*bytes.Buffer).String()
I'd prefer it if stderr and stdout were consistently typed...

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#newcode170
cmd/juju/status_test.go:170: "hardware": "arch=amd64 cpu-cores=1
mem=1024M",
Orthogonal... but this hardware data is not helpfully structured. Please
add a bug.

https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpress/metadata.yaml
File testing/repo/series/wordpress/metadata.yaml (right):

https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpress/metadata.yaml#newcode14
testing/repo/series/wordpress/metadata.yaml:14: scope: container
doesn't anything test for endpoints of wordpress elsewhere? If not,
cool, I'm just a bit surprised :).

https://codereview.appspot.com/12235043/

« Back to merge proposal