This is great, thanks. Modulo the /tmp-filesystem issue, this LGTM, on
the understanding that the local storage is not yet fit for purpose and
needs some concept of authentication [0] before we can actually let
people use this feature.

Let me know if I've completely misunderstood the overlap between
SSHStorage and LocalStorage, which is not a blocker for this CL but must
be resolved in a followup (if it's how I understood it to be).

[0] or to not be exposed, which we *might* be able to get away with if
we always used SSH storage from outside..?


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/12 07:59:00, axw1 wrote:
> 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.

Your mileage may vary :). I'm not really sold on either way tbh.

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#newcode172
environs/manual/bootstrap_test.go:172: args.MachineId = "1"
On 2013/09/12 07:59:00, axw1 wrote:
> 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.

Yeah, this isn't your problem. I think it still is a problem that we can
allow bootstrap with arbitrary ids, but that other bits of the system
assign special meaning to "0" (it somehow seems that every time we
eliminate a special case for 0 we end up with a new one somewhere else).

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/13635044/diff/14001/cmd/jujud/machine.go#newcode30
cmd/jujud/machine.go:30: localstorage
"launchpad.net/juju-core/worker/localstorage"
drop the explicit name?

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

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode45
environs/filestorage/filestorage.go:45: func NewFileStorage(path string)
(environs.Storage, error) {
High-level question while I think of it: does the SSHStorage on
bootstrap get reused but backed with a FileStorage? I sort of imagine
so, in which case, cool ...but is it possible for there to be overlap?
ie should we be sharing the flocking everywhere?

or am I just fantasizing?

https://codereview.appspot.com/13635044/diff/14001/environs/filestorage/filestorage.go#newcode110
environs/filestorage/filestorage.go:110: file, err :=
ioutil.TempFile("", "juju-filestorage-"+name)
Same different-filesystems problem.

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode28
environs/manual/bootstrap.go:28: bootstrap.BootstrapStorager
OK, I think my problem here is maybe that it's not *just* about
bootstrap storage, right? It's the same storage we always use, but this
is the path to it that's accessible from a client machine. Yes?

So, at least in the short term, we *will* need SSHStorage and
LocalStorage to coexist.

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go
File worker/localstorage/worker.go (left):

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go#oldcode1
worker/localstorage/worker.go:1: package storage
ah, I see. Can we call this localstorage please?

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