Code review comment for lp://staging/~wallyworld/juju-core/move-assignment-policy
Revision history for this message
| Ian Booth (wallyworld) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
9199653
demo site
(Get the code!)
Sorry for not being clear that the end goal is to use constraints for
the assignment policy. This was spelled out in Tim's pre-impl email.
The goal of this branch is to remove the hard coded policy values from
the providers. I envisioned that with constraints, there needs to be a
default value defined somewhere for when the constraint is not
specified. So I added that to the env, and use that to keep the system
working as before until constraints are written. And having it as an env
setting allows us as developers to actually use a different policy aside
from AssignNew so we can prototype and experiment etc.
https:/
File environs/
https:/
environs/
On 2013/05/28 12:16:48, fwereade wrote:
> A thought: should this be "clean" rather than "unused"?
This code is not new - it is copied directly from the state package. It
is moved here to avoid circular reference issues. I guess "clean" may be
a better name for it now given the new work we are doing. I'll get a
consensus from others before I make any change.
https:/
environs/
On 2013/05/28 12:16:48, fwereade wrote:
> How is this "default"? Surely it's just assignment-policy, unless
there's an
> alternative means of specifying it?
> Anyway, I'm really not sure it should go in the env config. How would
you feel
> about making it a constraint?
> (this opens up the default-
am aware,
> but it feels like the right time to have that discussion)
Perhaps my covering letter did not clearly enough described the reason
for this branch. The assignment policy *will* be a constraint when this
work is ultimately finished (this branch is an intermediate step).
However, there still needs to be a default behaviour for when the
constraint is not specified. Right now, each provider hard codes an
assignment policy. It shouldn't be done at the provider level. So what
we are doing here via the default-
the provider attribute and allowing the system to behave sensibly when
no constraint is specified (since that code is not written yet). I could
hard code juju to always use AssignNew until the constraints are done,
but why wouldn't we want to allow the user the option to specify a
default in the env? I wasn't aware this had been previously discussed.
With the code in this branch, the default-
optional to include in the env; if it's not there, AssignNew will be
used. But, the user can change that to AssignUnused if they choose to. I
haven't included it in the boilerplate config, so one currently needs to
know what one is doing to mess with it, but it sure will make things
easier for dev and testing.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:294: policy :=
conn.Environ.
On 2013/05/28 12:16:48, fwereade wrote:
> On 2013/05/28 11:37:20, jameinel wrote:
> > It seems you've moved the policy to environment config rather than
state. Or
> is
> > this Environ object instantiated from the state?
> >
> > The essential difference is whether you need to 'juju set' to get
new
> behavior,
> > or whether just editing ~/.juju/
client to
> > behave differently.
> This is definitely wrong.
> /me feels a little bit smug that the parallel non-matching environ
configs are
> causing exactly the confusion he predicted. It's less satisfying than
one might
> imagine.
The answer both reviewers' questions:
this is temporary until constraints are written. Because the policy is
no longer hard coded against the provider, I had to get it from
somewhere else (for now). Even with constraints, when they are written,
I'd love to understand any objections to allowing a default constraint
value to be specified in env. If a constraint is not specified, the
value to use has to come from somewhere. I thought environments.yaml was
a reasonable choice since it allows flexibility for dev and
experimentation etc.
There appeared to be a number of common env attributes, applicable to
all providers, so I stuck this new one there too. Hence to
conn.Environ.
https:/
File state/state.go (right):
https:/
state/state.go:944: case config.AssignLocal:
On 2013/05/28 12:16:48, fwereade wrote:
> This'll need some rearrangement if it's a constraint... but I haven't
yet seen
> anything to make me think it shouldn't be.
It definitely will be a constraint, but just not yet. This branch is
merely a step along the way. This branch removes the hard coded policy
values from the providers.
https:/