Code review comment for lp:~thumper/juju-core/juju-run-just-lock

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM with some consideration to my questions

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go
File cmd/jujud/run.go (right):

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go#newcode42
cmd/jujud/run.go:42: If --no-context is specified, the <unit-name>
positional
Not sure if --no-unit would be better here from a user perspective?

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run.go#newcode61
cmd/jujud/run.go:61: f.BoolVar(&c.noContext, "no-context", false, "do
not run the command in a unit context")
--no-unit???

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go
File cmd/jujud/run_test.go (right):

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go#newcode138
cmd/jujud/run_test.go:138: s.PatchValue(&fslock.LockWaitDelay,
10*time.Millisecond)
should this be ShortWait?

https://codereview.appspot.com/52470044/diff/1/cmd/jujud/run_test.go#newcode146
cmd/jujud/run_test.go:146: c.Assert(err, gc.ErrorMatches, "timeout")
What happens if this assert fails? The lock will still be held. Will
that adversely affect other tests? I think we should either add cleanup
or change the Assert to Check

https://codereview.appspot.com/52470044/

« Back to merge proposal