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

Thank you for the review. I have moved provider/local/storage to
worker/localstorage.

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 {
On 2013/09/11 13:51:21, fwereade wrote:
> Would that maybe be ".(BootstrapStorager)"?

Yes, that's a bit better, thanks.

> Elsewhere, we're experimenting with
> things like:

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

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

I'm not a big fan of the "HasX" style. IMO, the name should represent
the behaviour or role, not something an implementer has.

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)
On 2013/09/11 13:51:21, fwereade wrote:
> 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.

Done. It's a bit simpler here :)

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: }
On 2013/09/11 13:51:21, fwereade wrote:
> 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.

Sure. I've added some logger.Infofs to checkProvisioned,
detectSeriesAndHardwareCharacteristics and provisionMachineAgent. I've
purposely not added Errorfs, as error results will be logged upstream.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap.go#newcode146
environs/manual/bootstrap.go:146: // get the *state.Machine?
On 2013/09/11 13:51:21, fwereade wrote:
> Don't think so...

Oops! Nor do I. I had that in there from early on, and forgot to take it
out :)

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")
On 2013/09/11 13:51:21, fwereade wrote:
> FWIW, I'm kinda ok with panics for unexpected nils.

Noted.

https://codereview.appspot.com/13635044/diff/4001/environs/manual/bootstrap_test.go#newcode172
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
On 2013/09/11 13:51:21, fwereade wrote:
> 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?

The reason for this test is that Environ.Bootstrap is provided with a
machine ID to use. manual.Bootstrap is invoked via the null provider's
Bootstrap method. Thus, the test exists so as not to make assumptions
about being on machine "0".

Whether or not the counter is incremented... I don't think that can be
its concern, if it's been provided with the ID.

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

Done.

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()
On 2013/09/11 13:51:21, fwereade wrote:
> Why would we be getting multiple arches out of FindInstanceTools?
Aren't we
> passing one in?

Heh, yes. I probably copied that from somewhere. Fixed, thanks.

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