Code review comment for lp://staging/~axwalk/juju-core/wire-up-prechecker
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
5466644
demo site
(Get the code!)
I feel like we may be converging on something... let me know what you
think
https:/
File state/state.go (right):
https:/
state/state.go:292: // Prechecker is a clone of environs.
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/
> // 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:/
File cmd/juju/
https:/
cmd/juju/
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:/
File environs/
https:/
environs/
yeah, it seems sane to stick state.Prechecker in here and embed a shared
nil-returning implementation in the various implementations. Reasonable?
https:/
File juju/conn.go (right):
https:/
juju/conn.go:89: if prechecker, ok := environ.
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.EnablePre
that state doesn't know about environs and so we can't implement it
in-package.
Hmm:
type PrecheckerSource interface {
GetPrechec
}
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:/
File state/state.go (right):
https:/
state/state.go:70: precheckerMutex sync.Mutex
RWMutex might be appropriate here?
https:/
state/state.go:233: // we are creating a new machine instance (not a
container).
we should be locking these bits as well, surely?
https:/
File state/state_test.go (right):
https:/
state/state_
where did params come from?
https:/