Code review comment for lp://staging/~axwalk/juju-core/wire-up-prechecker
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
2eb648a
demo site
(Get the code!)
https:/
File cmd/juju/
https:/
cmd/juju/
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:/
File environs/
https:/
environs/
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:/
File juju/conn.go (right):
https:/
juju/conn.go:89: if prechecker, ok := environ.
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.EnablePre
> 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:/
File state/state.go (right):
https:/
state/state.go:70: precheckerMutex sync.Mutex
On 2013/10/01 10:24:33, fwereade wrote:
> RWMutex might be appropriate here?
Done.
https:/
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:/
File state/state_test.go (right):
https:/
state/state_
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:/