Code review comment for lp:~axwalk/juju-core/manual-bootstrap

Looking very nice; just a few questions, and a suggestion that you
consider moving the local storage worker into the top-level worker
package, rather than hiding it away under local.

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

https://codereview.appspot.com/13635044/diff/4001/cmd/juju/bootstrap.go#newcode71
cmd/juju/bootstrap.go:71: if bs, ok :=
environ.(bootstrap.BootstrapStorage); ok {
Would that maybe be ".(BootstrapStorager)"? Elsewhere, we're
experimenting with things like:

type HasBootstrapStorage interface {
     BootstrapStorage() (Storage, error)
}

...which IMO leads at least to more-readable casts. YMMV.

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/filestorage/filestorage.go#newcode114
environs/filestorage/filestorage.go:114: _, err = io.CopyN(file, r,
length)
As suggested in the previous review, please copy the whole lot somewhere
out of the way and move atomically into place on success; in this case I
think we've definitely vulnerable to errors with longer-term
consequences.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go
File environs/manual/bootstrap.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode143
environs/manual/bootstrap.go:143: }
This is really nice; can we produce some output while we're doing so? I
guess it'll have to be straight to the log for now, but a few INFO
messages would be helpful.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode146
environs/manual/bootstrap.go:146: // get the *state.Machine?
Don't think so...

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go
File environs/manual/bootstrap_test.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode153
environs/manual/bootstrap_test.go:153: c.Assert(Bootstrap(args),
gc.ErrorMatches, "Environ argument is nil")
FWIW, I'm kinda ok with panics for unexpected nils.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode172
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
Eyebrow slightly raised here... I'm not sure that's actually valid. Do
we initialize the machine id counter in state such that all subsequent
numbers are higher?

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode204
environs/manual/bootstrap_test.go:204: defer sshresponse(c,
checkProvisionedScript, "", 0)()
This code block is looking awfully familiar now. Can we tidy it up a
little?

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go
File environs/manual/tools.go (right):

https://codereview.appspot.com/13635044/diff/4001/environs/manual/tools.go#newcode23
environs/manual/tools.go:23: arches := possibleTools.Arches()
Why would we be getting multiple arches out of FindInstanceTools? Aren't
we passing one in?

https://codereview.appspot.com/13635044/