Code review comment for lp:~fwereade/juju-core/bootstrap-constraints-2

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

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#newcode63
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#newcode39
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#newcode185
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.

https://codereview.appspot.com/7610048/

« Back to merge proposal