cmd/juju: get-constraints, set-constraints

cmd/juju: get-constraints, set-constraints

Notable features:

  * Constraints parsing has been moved wholesale into state; this, and the
    associated test moves, make up the bulk of this diff, and don't involve
	logic changes in the actual parsing.
  * GetConstraintsCommand and SetConstraintsCommand are now implemented,
    and hooked up in Main().
  * Main() now registers commands in an order somewhat more readable than
    alphabetical, IMO.

https://code.launchpad.net/~fwereade/juju-core/cmd-get-set-constraints/+merge/148965

Requires: https://code.launchpad.net/~fwereade/juju-core/state-service-constraints/+merge/148944

(do not edit description out of merge proposal)

[fwereade, 2013-02-18 00:54:31]

Please take a look.

[dfc, 2013-02-18 01:32:24]

LGTM. I'm obviously not understanding something in parseConstraints.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode62
state/constraints.go:62: cons := &Constraints{}
why not

var cons Constraints

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode66
state/constraints.go:66: if err := cons.set(raw); err != nil {
return cons, err

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode71
state/constraints.go:71: return *cons, nil
return cons, nil

What am I missing ?

[dimitern, 2013-02-18 10:19:16]

LGTM, but I have some questions.

https://codereview.appspot.com/7323071/diff/1/cmd/juju/constraints_test.go
File cmd/juju/constraints_test.go (right):

https://codereview.appspot.com/7323071/diff/1/cmd/juju/constraints_test.go#ne...
cmd/juju/constraints_test.go:126: assertGet(c,
`{"cpu-cores":64,"cpu-power":0}`+"\n", "--format", "json")
When not setting a constraint, e.g. "--cpu-power=" shouldn't that be an error?

https://codereview.appspot.com/7323071/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode66
state/constraints.go:66: if err := cons.set(raw); err != nil {
On 2013/02/18 01:32:24, dfc wrote:
> return cons, err

+1

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode71
state/constraints.go:71: return *cons, nil
On 2013/02/18 01:32:24, dfc wrote:
> return cons, nil
> 
> What am I missing ?
+1

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode75
state/constraints.go:75: if raw == "" {
How about strings.Trim(raw, " ") first?

https://codereview.appspot.com/7323071/diff/1/state/constraints_test.go
File state/constraints_test.go (right):

https://codereview.appspot.com/7323071/diff/1/state/constraints_test.go#newco...
state/constraints_test.go:102: args:    []string{"mem="},
Not sure mem= or mem=0 is a valid constraint. Care to explain?

[rog, 2013-02-18 10:19:58]

LGTM with a few minor points and queries below.

https://codereview.appspot.com/7323071/diff/1/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/7323071/diff/1/cmd/juju/main.go#newcode30
cmd/juju/main.go:30: // Register creation commands.
i'm not sure about these changes.
alphabetical order makes it very easy to see whether a given command is included
in the list or not. if there's a place for categorisation, it's probably in the
help output.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode61
state/constraints.go:61: func ParseConstraints(args ...string) (Constraints,
error) {
thanks for moving this here.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode62
state/constraints.go:62: cons := &Constraints{}
On 2013/02/18 01:32:24, dfc wrote:
> why not
> 
> var cons Constraints

+1

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode66
state/constraints.go:66: if err := cons.set(raw); err != nil {
On 2013/02/18 01:32:24, dfc wrote:
> return cons, err

-1
i think it's nicer to see an obvious zero value returned.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode71
state/constraints.go:71: return *cons, nil
On 2013/02/18 01:32:24, dfc wrote:
> return cons, nil
> 
> What am I missing ?

+1

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode74
state/constraints.go:74: func (c *Constraints) set(raw string) (err error) {
no need to declare the err return AFAICS

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode156
state/constraints.go:156: // constraintsDoc is the mongodb representation of a
Constraints.
out of interest, why not just use Constraints directly?

[fwereade, 2013-02-20 14:55:16]

*** Submitted:

cmd/juju: get-constraints, set-constraints

Notable features:

  * Constraints parsing has been moved wholesale into state; this, and the
    associated test moves, make up the bulk of this diff, and don't involve
	logic changes in the actual parsing.
  * GetConstraintsCommand and SetConstraintsCommand are now implemented,
    and hooked up in Main().
  * Main() now registers commands in an order somewhat more readable than
    alphabetical, IMO.

R=dfc, dimitern, rog
CC=
https://codereview.appspot.com/7323071

https://codereview.appspot.com/7323071/diff/1/cmd/juju/constraints_test.go
File cmd/juju/constraints_test.go (right):

https://codereview.appspot.com/7323071/diff/1/cmd/juju/constraints_test.go#ne...
cmd/juju/constraints_test.go:126: assertGet(c,
`{"cpu-cores":64,"cpu-power":0}`+"\n", "--format", "json")
On 2013/02/18 10:19:16, dimitern wrote:
> When not setting a constraint, e.g. "--cpu-power=" shouldn't that be an error?

I see no "--cpu-power" anywhere ;p. "cpu-power=" indicates that any value is
acceptable.

https://codereview.appspot.com/7323071/diff/1/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/7323071/diff/1/cmd/juju/main.go#newcode30
cmd/juju/main.go:30: // Register creation commands.
On 2013/02/18 10:19:58, rog wrote:
> i'm not sure about these changes.
> alphabetical order makes it very easy to see whether a given command is
included
> in the list or not. if there's a place for categorisation, it's probably in
the
> help output.

The issue here is partly that I found the big block of code unhelpfully heavy on
the eyes, and partly that I found it very hard to navigate -- it's really not
that convenient to have Expose/Unexpose, and all the Set/Gets, far from their
counterparts. Wrt add/destroy, at least the complementary blocks are next to one
another (which I think is ok because there's no strict 1:1 correspondence any
more).

Independently, I think I'm +1 on categorization in the output.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode61
state/constraints.go:61: func ParseConstraints(args ...string) (Constraints,
error) {
On 2013/02/18 10:19:58, rog wrote:
> thanks for moving this here.

yw :)

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode62
state/constraints.go:62: cons := &Constraints{}
On 2013/02/18 10:19:58, rog wrote:
> On 2013/02/18 01:32:24, dfc wrote:
> > why not
> > 
> > var cons Constraints
> 
> +1

Heh, because I wasn't actually consciously aware of the
pointer-receiver-methods-work-on-addressable-values magic. Not 100% sure I
approve, but done.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode66
state/constraints.go:66: if err := cons.set(raw); err != nil {
On 2013/02/18 10:19:58, rog wrote:
> On 2013/02/18 01:32:24, dfc wrote:
> > return cons, err
> 
> -1
> i think it's nicer to see an obvious zero value returned.

And I also think it's important to return an actual zero value.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode71
state/constraints.go:71: return *cons, nil
On 2013/02/18 10:19:58, rog wrote:
> On 2013/02/18 01:32:24, dfc wrote:
> > return cons, nil
> > 
> > What am I missing ?
> 
> +1

Done.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode74
state/constraints.go:74: func (c *Constraints) set(raw string) (err error) {
On 2013/02/18 10:19:58, rog wrote:
> no need to declare the err return AFAICS

Done.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode75
state/constraints.go:75: if raw == "" {
On 2013/02/18 10:19:16, dimitern wrote:
> How about strings.Trim(raw, " ") first?

Don't think there's any need to do that considering the only client is
ParseConstraints; but I renamed this to setRaw and documented it to take
name=value, and moved the empty check up to ParseConstraints.

https://codereview.appspot.com/7323071/diff/1/state/constraints.go#newcode156
state/constraints.go:156: // constraintsDoc is the mongodb representation of a
Constraints.
On 2013/02/18 10:19:58, rog wrote:
> out of interest, why not just use Constraints directly?

I think it's a mistake to conflate the types we need to store with the types we
expose in the public API -- consider (eg) a machine or unit document as exposed
by the API; it's not the same as one in the database. While there's no strict
need to separate them at the moment, I'd rather pay the cost of an extra type to
keep the distinction explicit.

https://codereview.appspot.com/7323071/diff/1/state/constraints_test.go
File state/constraints_test.go (right):

https://codereview.appspot.com/7323071/diff/1/state/constraints_test.go#newco...
state/constraints_test.go:102: args:    []string{"mem="},
On 2013/02/18 10:19:16, dimitern wrote:
> Not sure mem= or mem=0 is a valid constraint. Care to explain?

"mem=": "I don't care how much memory I get"
"mem=0": "I demand at least 0MB of memory"

They are functionally equivalent.