Merge lp:~fwereade/pyjuju/actual-provider-constraints into lp:~fwereade/pyjuju/shadow-trunk-1204

[William Reade]

Providers are now directly responsible for their own constraints

This is really just a rearrangement of code; nothing fundamental has
actually changed, except that Constraints.from_strs is entirely gone; and
trying to construct Constraints(None, data) will no longer produce a viable
specimen.

https://codereview.appspot.com/5939046/

[William Reade (fwereade)]

Reviewers: mp+99655_code.launchpad.net,

Message:
Please take a look.

Description:
Providers are now directly responsible for their own constraints

This is really just a rearrangement of code; nothing fundamental has
actually changed, except that Constraints.from_strs is entirely gone;
and
trying to construct Constraints(None, data) will no longer produce a
viable
specimen.

https://code.launchpad.net/~fwereade/juju/actual-provider-constraints/+merge/99655

Requires:
https://code.launchpad.net/~fwereade/juju/retire-direct-constraints-creation/+merge/99651

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5939046/

Affected files:
   A [revision details]
   M juju/machine/constraints.py
   M juju/machine/tests/test_constraints.py
   M juju/providers/common/base.py
   M juju/providers/ec2/__init__.py
   M juju/providers/ec2/tests/test_provider.py
   M juju/providers/ec2/utils.py
   M juju/providers/orchestra/__init__.py
   M juju/providers/orchestra/tests/test_provider.py

[lp:~fwereade/pyjuju/actual-provider-constraints]

509. By William Reade on 2012-03-28

merge parent

510. By William Reade on 2012-03-28

allow 'any' to set a None value on any constraint; don't add conflict overlays agains None values

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/5939046/

[lp:~fwereade/pyjuju/actual-provider-constraints]

511. By William Reade on 2012-03-28

ec2 now correctly defaults back to amd64 if arch is explicitly unset

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/5939046/

[Kapil Thangavelu (hazmat)]

LGTM, thanks for working on this.

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#newcode170
juju/machine/constraints.py:170: if data[name] is None:
Doesn't this value need to be kept to properly compose constraints?

ie say we had env constraints with an instance-type=m1.large, and cli
constraints for the service with instance-type=any... we'll end up using
the env constraints, i guess i can see reasoning both ways, but any at
that point means use default.. which doesn't sound quite right.

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py
File juju/providers/ec2/utils.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py#newcode57
juju/providers/ec2/utils.py:57: if s not in _PLAUSIBLE_ZONES:
nitpick, any reason to just not lower the string here, ie. be accepting
of nominally valid inputs.

https://codereview.appspot.com/5939046/

[William Reade (fwereade)]

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#newcode170
juju/machine/constraints.py:170: if data[name] is None:
On 2012/03/28 17:29:04, hazmat wrote:
> Doesn't this value need to be kept to properly compose constraints?

> ie say we had env constraints with an instance-type=m1.large, and cli
> constraints for the service with instance-type=any... we'll end up
using the env
> constraints, i guess i can see reasoning both ways, but any at that
point means
> use default.. which doesn't sound quite right.

I think you've misunderstood (...or maybe I have ;)).

instance-type=m1.large overwritten with instance-type=any will actually
give you an amd64 t1.micro; the cpu/mem constraints at the m1.large
level are cleared by dint of being next to an explicitly-set m1.large,
and then the m1.large is also cleared out by the instance-type=any.

The kicker is cpu=10 mem=8G overwritten with instance-type=any, which
IMO should absolutely retain the cpu=10 mem=8G (and won't, if the later
instance-type-unsetting also unsets everything it conflicts with --
which is what this change fixes).

Incidentally, "any" only means "use default" when the default is None;
"" always means "use default".

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py
File juju/providers/ec2/utils.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/providers/ec2/utils.py#newcode57
juju/providers/ec2/utils.py:57: if s not in _PLAUSIBLE_ZONES:
On 2012/03/28 17:29:04, hazmat wrote:
> nitpick, any reason to just not lower the string here, ie. be
accepting of
> nominally valid inputs.

Eh, why not :). Done.

https://codereview.appspot.com/5939046/

[lp:~fwereade/pyjuju/actual-provider-constraints]

512. By William Reade on 2012-03-28

merge parent

513. By William Reade on 2012-03-28

address review

[Kapil Thangavelu (hazmat)]

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#newcode170
juju/machine/constraints.py:170: if data[name] is None:
On 2012/03/28 17:58:11, fwereade wrote:
> On 2012/03/28 17:29:04, hazmat wrote:
> > Doesn't this value need to be kept to properly compose constraints?
> >
> > ie say we had env constraints with an instance-type=m1.large, and
cli
> > constraints for the service with instance-type=any... we'll end up
using the
> env
> > constraints, i guess i can see reasoning both ways, but any at that
point
> means
> > use default.. which doesn't sound quite right.

> I think you've misunderstood (...or maybe I have ;)).

> instance-type=m1.large overwritten with instance-type=any will
actually give you
> an amd64 t1.micro; the cpu/mem constraints at the m1.large level are
cleared by
> dint of being next to an explicitly-set m1.large, and then the
m1.large is also
> cleared out by the instance-type=any.

> The kicker is cpu=10 mem=8G overwritten with instance-type=any, which
IMO should
> absolutely retain the cpu=10 mem=8G (and won't, if the later
> instance-type-unsetting also unsets everything it conflicts with --
which is
> what this change fixes).

> Incidentally, "any" only means "use default" when the default is None;
"" always
> means "use default".

if the any value isn't stored then when composing constraints from
different levels, there isn't any distinguishing between something which
was set and something that was set to 'any'. granted we don't compose
multi-level constraints right now, but given env constraints, this would
need changing to keep the recorded value for comparision afaics.

https://codereview.appspot.com/5939046/

[William Reade (fwereade)]

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5939046/diff/4001/juju/machine/constraints.py#newcode170
juju/machine/constraints.py:170: if data[name] is None:
On 2012/03/28 18:07:39, hazmat wrote:
> On 2012/03/28 17:58:11, fwereade wrote:
> > On 2012/03/28 17:29:04, hazmat wrote:
> > > Doesn't this value need to be kept to properly compose
constraints?
> > >
> > > ie say we had env constraints with an instance-type=m1.large, and
cli
> > > constraints for the service with instance-type=any... we'll end up
using the
> > env
> > > constraints, i guess i can see reasoning both ways, but any at
that point
> > means
> > > use default.. which doesn't sound quite right.
> >
> > I think you've misunderstood (...or maybe I have ;)).
> >
> > instance-type=m1.large overwritten with instance-type=any will
actually give
> you
> > an amd64 t1.micro; the cpu/mem constraints at the m1.large level are
cleared
> by
> > dint of being next to an explicitly-set m1.large, and then the
m1.large is
> also
> > cleared out by the instance-type=any.
> >
> > The kicker is cpu=10 mem=8G overwritten with instance-type=any,
which IMO
> should
> > absolutely retain the cpu=10 mem=8G (and won't, if the later
> > instance-type-unsetting also unsets everything it conflicts with --
which is
> > what this change fixes).
> >
> > Incidentally, "any" only means "use default" when the default is
None; ""
> always
> > means "use default".

> if the any value isn't stored then when composing constraints from
different
> levels, there isn't any distinguishing between something which was set
and
> something that was set to 'any'. granted we don't compose multi-level
> constraints right now, but given env constraints, this would need
changing to
> keep the recorded value for comparision afaics.

For the record, I was unclear: this loop only deals with masking out
conflicts, the unset value still gets stored below.

https://codereview.appspot.com/5939046/

[lp:~fwereade/pyjuju/actual-provider-constraints]

514. By William Reade on 2012-03-28

merge parent

[William Reade (fwereade)]

*** Submitted:

Providers are now directly responsible for their own constraints

This is really just a rearrangement of code; nothing fundamental has
actually changed, except that Constraints.from_strs is entirely gone;
and
trying to construct Constraints(None, data) will no longer produce a
viable
specimen.

R=hazmat
CC=
https://codereview.appspot.com/5939046

https://codereview.appspot.com/5939046/