I feel like we may be converging on something... let me know what you
think


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

https://codereview.appspot.com/14032043/diff/1/state/state.go#newcode292
state/state.go:292: // Prechecker is a clone of environs.Prechecker,
here to avoid the dependency.
On 2013/09/29 16:20:44, axw1 wrote:
> On 2013/09/27 10:59:22, fwereade wrote:
> > Better I think to define it in state, and document (in code) that
appropriate
> > Environs implement it. environs already knows about state, not vice
versa, so
> we
> > may as well just define the interface in the place that uses it.
After all, it
> > really *doesn't* have to be just an environ that satsisfies that
interface:
> we'd
> > surely want the state tests to be written with a suitable test
implementation,
> > not of a whole environ.

> Fair enough, I'll do that. Do you think it'd be worthwhile adding
something like
> below to environs/interface.go?

> // Prechecker is an optional Environ interface. See state.Prechecker
for
> details.
> type Prechecker interface {
>      state.Prechecker
> }

I think rog and nate have convinced me that these optional interfaces
may do more harm than good -- we can add state.Prechecker directly to
the Environ interface, and just supply an embeddable do-nothing
prechecker that people can implement to begin with.

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)
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.

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 {
yeah, it seems sane to stick state.Prechecker in here and embed a shared
nil-returning implementation in the various implementations. Reasonable?

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 {
It's that old wrong-environ problem again, I'm afraid. 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
RWMutex might be appropriate here?

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).
we should be locking these bits as well, surely?

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 = ""
where did params come from?

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