https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/14032043/diff/6001/cmd/juju/addmachine.go#newcode98
cmd/juju/addmachine.go:98: conn.State.SetPrechecker(nil)
On 2013/10/01 10:24:33, fwereade wrote:
> This feels like a timebomb to me. What happens when this functionality
moves
> into the API server? One manually provisioned machine will disable
prechecking
> for everything.

> I think we should probably be skipping prechecks on injected machines
anyway.

Removed, and updated state to not call PrecheckInstance for injected
machines.

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/14032043/diff/6001/environs/interface.go#newcode105
environs/interface.go:105: type Environ interface {
On 2013/10/01 10:24:33, fwereade wrote:
> yeah, it seems sane to stick state.Prechecker in here and embed a
shared
> nil-returning implementation in the various implementations.
Reasonable?

Very. I've created a new file, base.go, which contains a default
implementation.

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/6001/juju/conn.go#newcode89
juju/conn.go:89: if prechecker, ok := environ.(state.Prechecker); ok {
On 2013/10/01 10:24:33, fwereade wrote:
> It's that old wrong-environ problem again, I'm afraid.

So, my intention here was that the user would later update the environ.
But actually, thinking about it some more, this is just wrong. NewConn
shouldn't call SetPrechecker. If the user wants to do that, they can.

The one user that perhaps should SetPrechecker is NewConnFromName. Is
there any reason why we don't have it always set the environ's config
from state?

> I'd reuse that utility
> function from jujud myself -- I think it's basically
State.EnablePrechecking, or
> something like that, with the complication that state doesn't know
about
> environs and so we can't implement it in-package.

> Hmm:

> type PrecheckerSource interface {
>      GetPrechecker(st *State) (Prechecker, error)
> }

> how often do we directly Open state? often enough that supplying one
of those
> would be an inconvenience? Because if we could do that, we could drop
the
> locking in here, I think, and rely on safe/efficient external
implementations of
> GetPrechecker.

https://codereview.appspot.com/14032043/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode70
state/state.go:70: precheckerMutex  sync.Mutex
On 2013/10/01 10:24:33, fwereade wrote:
> RWMutex might be appropriate here?

Done.

https://codereview.appspot.com/14032043/diff/6001/state/state.go#newcode233
state/state.go:233: // we are creating a new machine instance (not a
container).
On 2013/10/01 10:24:33, fwereade wrote:
> we should be locking these bits as well, surely?

Indeed, thanks for picking that up. Done.

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/14032043/diff/6001/state/state_test.go#newcode1951
state/state_test.go:1951: params.ParentId = ""
On 2013/10/01 10:24:33, fwereade wrote:
> where did params come from?

Derp. I split the test above; thought I ran the tests, but clearly did
not.

https://codereview.appspot.com/14032043/