Code review comment for lp:~rogpeppe/juju-core/mfoord-wrapsingletonworkers

Revision history for this message
Nate Finch (natefinch) wrote :

LGTM with just a couple minor suggestions.

https://codereview.appspot.com/86200043/diff/40001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/86200043/diff/40001/cmd/jujud/machine.go#newcode253
cmd/jujud/machine.go:253: conn := singularAPIConn{st, st.Agent()}
could this code get moved to the loop over entity.jobs below? It
diesn't seem to neec to be here, and it puts the declaration far from
the use of the variable.

https://codereview.appspot.com/86200043/diff/40001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/86200043/diff/40001/cmd/jujud/machine_test.go#newcode384
cmd/jujud/machine_test.go:384: c.Assert(s.singularRecord.started(),
jc.DeepEquals, []string{
This checks order of the slice, too, right? Does the order matter? If
not, we may want to use jc.SameContents here, so we don't fail the test
if the order changes.

https://codereview.appspot.com/86200043/

« Back to merge proposal