cmd/juju: bootstrap accepts --constraints

cmd/juju: bootstrap accepts --constraints

...and passes them on to environs.Bootstrap, which passes them on to
Environ.Bootstrap. Only the dummy provider actually handles constraints
at the moment -- so we can test the bootstrap command -- all the others
will be implemented in followups.

https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-2/+merge/153600

Requires: https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-1/+merge/153523

(do not edit description out of merge proposal)

[fwereade, 2013-03-15 17:09:57]

Please take a look.

[dimitern, 2013-03-15 17:33:20]

LGTM, with some suggestions/questions.

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

https://codereview.appspot.com/7610048/diff/1/cmd/juju/bootstrap.go#newcode29
cmd/juju/bootstrap.go:29: f.Var(state.ConstraintsValue{&c.Constraints},
"constraints", "set environment constraints")
This was included in the prereq CL with a slightly different wording - won't it
result in a merge conflict?

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

https://codereview.appspot.com/7610048/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:63: defer testing.MakeFakeHome(c, envConfig,
"brokenenv").Restore()
this is iffy and kinda confusing - what's broken about it?

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap.go#newcode22
environs/bootstrap.go:22: // uploaded, as documented in Environ.Bootstrap.
please, update the comment to include the constraints arg.

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:39: err := environs.Bootstrap(env,
state.Constraints{}, false, nil)
couldn't this be *state.Constraints instead, so we can pass nil? (elsewhere as
well)

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:185: func (e *bootstrapEnviron) Bootstrap(cons
state.Constraints, uploadTools bool, certPEM, keyPEM []byte) error {
see above

https://codereview.appspot.com/7610048/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/interface.go#newcode151
environs/interface.go:151: Bootstrap(cons state.Constraints, uploadTools bool,
stateServerCert, stateServerKey []byte) error
please update the comment as well.

[jameinel, 2013-03-18 18:04:58]

LGTM

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:39: err := environs.Bootstrap(env,
state.Constraints{}, false, nil)
+1, as it also means the struct doesn't get copied when it doesn't need to.
Though I suppose we want to avoid the side-effect of passing in constraints and
then changing the in-memory struct later?
But that could just be the final e.constraints = *cons (so it does do an
explicit copy at that point).

[dfc, 2013-03-18 22:54:54]

LGTM.

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:39: err := environs.Bootstrap(env,
state.Constraints{}, false, nil)
On 2013/03/18 18:04:58, jameinel wrote:
> +1, as it also means the struct doesn't get copied when it doesn't need to.
> Though I suppose we want to avoid the side-effect of passing in constraints
and
> then changing the in-memory struct later?
> But that could just be the final e.constraints = *cons (so it does do an
> explicit copy at that point).

I've pondered on this as well, passing a struct is less common than a pointer to
a struct, but in this case I think it is correct, if a little unorthodox.

By creating a new empty (zero value) constraint set here we guarantee that it
can't be mutated afterwards by the caller. This is a useful property for
something as contentious as constraints.

[thumper, 2013-03-18 23:07:54]

On 2013/03/18 22:54:54, dfc wrote:
> LGTM.
> 
> https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go
> File environs/bootstrap_test.go (right):
> 
>
https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go#newc...
> environs/bootstrap_test.go:39: err := environs.Bootstrap(env,
> state.Constraints{}, false, nil)
> On 2013/03/18 18:04:58, jameinel wrote:
> > +1, as it also means the struct doesn't get copied when it doesn't need to.
> > Though I suppose we want to avoid the side-effect of passing in constraints
> and
> > then changing the in-memory struct later?
> > But that could just be the final e.constraints = *cons (so it does do an
> > explicit copy at that point).
> 
> I've pondered on this as well, passing a struct is less common than a pointer
to
> a struct, but in this case I think it is correct, if a little unorthodox.
> 
> By creating a new empty (zero value) constraint set here we guarantee that it
> can't be mutated afterwards by the caller. This is a useful property for
> something as contentious as constraints.

This is about to conflict wonderfully with the branch I'm about to merge.

[fwereade, 2013-03-19 10:30:36]

*** Submitted:

cmd/juju: bootstrap accepts --constraints

...and passes them on to environs.Bootstrap, which passes them on to
Environ.Bootstrap. Only the dummy provider actually handles constraints
at the moment -- so we can test the bootstrap command -- all the others
will be implemented in followups.

R=dimitern, jameinel, dfc, thumper
CC=
https://codereview.appspot.com/7610048

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

https://codereview.appspot.com/7610048/diff/1/cmd/juju/bootstrap.go#newcode29
cmd/juju/bootstrap.go:29: f.Var(state.ConstraintsValue{&c.Constraints},
"constraints", "set environment constraints")
On 2013/03/15 17:33:20, dimitern wrote:
> This was included in the prereq CL with a slightly different wording - won't
it
> result in a merge conflict?

That was jujud, not juju

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

https://codereview.appspot.com/7610048/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:63: defer testing.MakeFakeHome(c, envConfig,
"brokenenv").Restore()
On 2013/03/15 17:33:20, dimitern wrote:
> this is iffy and kinda confusing - what's broken about it?

Dropped.

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap.go#newcode22
environs/bootstrap.go:22: // uploaded, as documented in Environ.Bootstrap.
On 2013/03/15 17:33:20, dimitern wrote:
> please, update the comment to include the constraints arg.

Done.

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:39: err := environs.Bootstrap(env,
state.Constraints{}, false, nil)
On 2013/03/18 22:54:54, dfc wrote:
> On 2013/03/18 18:04:58, jameinel wrote:
> > +1, as it also means the struct doesn't get copied when it doesn't need to.
> > Though I suppose we want to avoid the side-effect of passing in constraints
> and
> > then changing the in-memory struct later?
> > But that could just be the final e.constraints = *cons (so it does do an
> > explicit copy at that point).
> 
> I've pondered on this as well, passing a struct is less common than a pointer
to
> a struct, but in this case I think it is correct, if a little unorthodox.
> 
> By creating a new empty (zero value) constraint set here we guarantee that it
> can't be mutated afterwards by the caller. This is a useful property for
> something as contentious as constraints.
> 

Value semantics are useful; there's no reason to use nil when there's a
perfectly good zero value; and I don't believe the overhead of copying these
structs around a bit is going to hurt us.

https://codereview.appspot.com/7610048/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7610048/diff/1/environs/interface.go#newcode151
environs/interface.go:151: Bootstrap(cons state.Constraints, uploadTools bool,
stateServerCert, stateServerKey []byte) error
On 2013/03/15 17:33:20, dimitern wrote:
> please update the comment as well.

Done.