New constraints validation and merge

New constraints validation and merge

A new constraints Validator implementation
is introduced, which provides validation
and merging support for constraints. It
allows for conflicting attributes and 
unsupported attributes to be registered,
and does the right thing with these. 
To illustrate the point, a new constraints
attribute instance-type is also added, which
will be used in a followup branch.

https://code.launchpad.net/~wallyworld/juju-core/constraints-validation-merge/+merge/215807

(do not edit description out of merge proposal)

[wallyworld, 2014-04-15 05:33:39]

Please take a look.

[wallyworld, 2014-04-16 01:09:02]

Please take a look.

[wallyworld, 2014-04-16 03:30:54]

Please take a look.

[wallyworld, 2014-04-16 03:49:29]

Please take a look.

[fwereade, 2014-04-18 07:48:20]

LGTM apart from the overly-broad interface.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go
File constraints/validation.go (right):

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#...
constraints/validation.go:17: type Validator interface {
I'm not sure why this is an interface when it's impossible for other people to
implement it (unexported methods...). In general, I'm all for interfaces at
judiciously chosen boundaries, but I'm not 100% sure this is one. Something
like:

type Validator interface {
    Validate(cons Value) error
    Merge(consA, consB Value) (Value, error)
}

...would feel neater to me. Although "consA, consB" is suboptimal... "fallbacks,
primary" doesn't work either, but I'd really like something about the names to
indicate which is overriding which.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#...
constraints/validation.go:25: RegisterConflicts(reds, blues []string)
I really appreciate blank lines before comments.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#...
constraints/validation.go:137: return v.checkUnsupported(cons)
Callers will indeed probably want to ignore them. I wonder how we can cleanly
feed this stuff back to CLI users (or, well, any users)?

[wallyworld, 2014-04-18 13:10:57]

Please take a look.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go
File constraints/validation.go (right):

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#...
constraints/validation.go:17: type Validator interface {
On 2014/04/18 07:48:20, fwereade wrote:
> I'm not sure why this is an interface when it's impossible for other people to
> implement it (unexported methods...). In general, I'm all for interfaces at
> judiciously chosen boundaries, but I'm not 100% sure this is one. Something
> like:
> 
> type Validator interface {
>     Validate(cons Value) error
>     Merge(consA, consB Value) (Value, error)
> }
> 
> ...would feel neater to me. Although "consA, consB" is suboptimal...
"fallbacks,
> primary" doesn't work either, but I'd really like something about the names to
> indicate which is overriding which.

I renamed consA to consFallbacks, and consB to cons
I kept only the exported methods on the interface. This includes both the
Register methods and Validate and Merge.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#...
constraints/validation.go:25: RegisterConflicts(reds, blues []string)
On 2014/04/18 07:48:20, fwereade wrote:
> I really appreciate blank lines before comments.

Done.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#...
constraints/validation.go:137: return v.checkUnsupported(cons)
On 2014/04/18 07:48:20, fwereade wrote:
> Callers will indeed probably want to ignore them. I wonder how we can cleanly
> feed this stuff back to CLI users (or, well, any users)?

I reworked the Validate interface - it now returns ([]string, error).
[]string is the unsupported attributes, error is the true validation error. The
NotSUpportedError is gone. So if error is nil, the caller can then see if the
unsupported attrs is populated and do as it wishes, or just call _, err :=
validator.Validate(....)

[fwereade, 2014-04-21 10:34:14]

Still LGTM, with a vague feeling that an explicit UnsupportedConstraints error,
with a method that itself returns the []string, would be even nicer -- and then
it's a similar story, you try to validate and can either ignore or handle the
specific error as you please. I'll leave it to your judgment, though.