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

Revision history for this message
Tim Penhey (thumper) wrote :

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
On 2014/01/15 02:03:18, wallyworld wrote:
> Not sure if --no-unit would be better here from a user perspective?

Users don't see this. This is only really used by the juju run
apiserver component.

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)
On 2014/01/15 02:03:18, wallyworld wrote:
> should this be ShortWait?

No, I wanted to make sure that it checked faster than short wait,
otherwise there was the potential that after unlocking, it didn't have
enough time to execute fully.

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")
On 2014/01/15 02:03:18, wallyworld wrote:
> 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

No, the lock is in the LockDir, which is mocked out above in a temporary
test directory.

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

« Back to merge proposal