Code review comment for lp:~axwalk/juju-core/wire-up-prechecker-take2

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

LGTM, thanks

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/61520045/diff/20001/cmd/jujud/bootstrap_test.go#newcode121
cmd/jujud/bootstrap_test.go:121: }, state.DefaultDialOpts(),
state.Policy(nil))
On 2014/02/19 07:08:45, axw wrote:
> In any case, juju-core/agent can't use it directly. The policy
implementation
> requires environs, which depends on agent (which seems a bit
unfortunate.)

Grar. Navigate the twisty passages as you must, then. Cheers.

> No problems. I'd prefer to keep the ones in juju-core/state
self-contained
> though.

SGTM

https://codereview.appspot.com/61520045/diff/40001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/61520045/diff/40001/state/policy.go#newcode58
state/policy.go:58: logger.Debugf("policy returned nil prechecker,
ignoring")
I think this is really bad behaviour on the policy's part; probably bad
enough to justify erroring out of the AddMachine, because the
implementation is crazy enough that all bets should be considered to be
off.

https://codereview.appspot.com/61520045/

« Back to merge proposal