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/