Code review comment for lp:~axwalk/juju-core/wire-up-prechecker-take2
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Please take a look.
https:/
File cmd/jujud/
https:/
cmd/jujud/
state.Policy(nil))
On 2014/02/18 15:30:18, fwereade wrote:
> I'm wondering whether it might be good to have a package that
implements
> environStatePolicy, and includes a state.Open wrapper that always
passes the
> environ policy in. I'm not sure I can see a use case for alternative
policies at
> the moment, and if one does show up I think it'll be pretty easy to
add.
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.)
I'll just move the policy implementation into environs. The wrappers
aren't really all that useful since they can't be used by both
production and testing code.
> As it is I feel it's a bit icky having the nil policies everywhere --
I think we
> probably want all our states to work the same in-test and out. Open to
> counterarguments though.
No problems. I'd prefer to keep the ones in juju-core/state
self-contained though.
https:/
File cmd/jujud/
https:/
cmd/jujud/
On 2014/02/18 15:30:18, fwereade wrote:
> I don't really like the nil-return-
specific sort of
> NotImplemented error that we handle explicitly in precheckInstance,
please?
Done.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:57: policy := state.PolicyBase{}
On 2014/02/18 15:30:18, fwereade wrote:
> This bit doesn't have to use the mooted wrapper, ofc.
> *but* I don't quite understand what the benefit is of having a null
policy here.
> What breaks down if we use a real one?
Nothing actually, this is basically a holdover from when Environ had to
be updated.
I'll move the policy implementation to its own package as suggested and
update this.
https:/
File state/policy.go (right):
https:/
state/policy.go:17: // for any of the interfaces, but should only return
On 2014/02/18 15:30:18, fwereade wrote:
> I think you a word.
Done.
https:/
state/policy.go:43: return nil, nil
On 2014/02/18 15:30:18, fwereade wrote:
> yeah, `nil, nil` scares me. Please change it :).
Done. Must return an error that satisfies errors.
https:/