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

Still some changes needed here I think

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

https://codereview.appspot.com/13635044/diff/14001/cmd/juju/bootstrap.go#newcode76
cmd/juju/bootstrap.go:76: environ = &bootstrapStorageEnviron{environ,
bootstrapStorage}
If all we are caring about is the storage, why are we changing the
environ?

Does it not make sense to change the things that depend on the storage
to just depend on the storage and not the entire environment, then you
don't need to mock out the Storage method.

Obviously here the important thing is that we want to use a particular
storage for bootstrap, so I'd expect to be assigning the
bootstrapStorage to a storage variable, not mocking out the method with
a different structure.

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

https://codereview.appspot.com/13635044/diff/14001/environs/bootstrap/bootstrap.go#newcode24
environs/bootstrap/bootstrap.go:24: // BootstrapStorager is an interface
that returns a environs.Storage that may
On 2013/09/14 07:28:03, rog wrote:
> I think I'd define this inside environs/interface.go

> This package doesn't define or use it, and that file
> is a useful reference for people implementing providers.

Agreed. It is something that is used by the environment, so it makes
more sense to live there.

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#newcode110
environs/filestorage/filestorage.go:110: file, err :=
ioutil.TempFile("", "juju-filestorage-"+name)
On 2013/09/13 09:41:26, axw1 wrote:
> On 2013/09/13 09:24:59, fwereade wrote:
> > Same different-filesystems problem.

> This is only for testing, but I will fix it anyway. I think
localstorage ought
> to be renamed httpstorage, and have the backend contain one of these.

That sounds like a good idea IMO

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/agent.go#newcode82
environs/manual/agent.go:82: err := environs.FinishMachineConfig(mcfg,
args.envcfg, constraints.Value{})
I fear that using names like "envcfg" means that we have more and more
assumed knowledge encoded into our variable names. Here I like some
parts of the Zen of Python "Explicit is better than implicit", yes it is
python, but I think the principle holds.

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
Arg, here I know that I'm going to clash on names with Rog, but IMO
   BootstrapStorager
is a terrible name.
   HasBootstrapStorage
makes a lot more sense to me. Firstly you can look at the name and know
what is going on, and we aren't inventing words to go by the weird idea
that interfaces should end with "er".

bootstrapStorage, ok := sometype.(HasBootstrapStorage)

reads ok to me (FSVO ok given the syntax of the type assertion).

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshresponse_test.go#newcode53
environs/manual/sshresponse_test.go:53: return
testing.PatchEnvironment("PATH", fakebin+":"+os.Getenv("PATH"))
I've been thinking of better ways to get test code to clean up after
itself.

If we had an interface called:
   type TestCleanup interface {
     AddCleanup(func())

we could then have this function take an interface pointer,
and instead of returning a function that the caller needs to defer,
you could just go:

func sshresponse(c *gc.C, cleaner testing.TestCleanup, input, output
string, rc int) {
   // everything else
   cleaner.AddCleanup(testing.PatchEnvironment("PATH",
fakebin+":"+os.Getenv("PATH")))
}

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

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/config.go#newcode4
worker/localstorage/config.go:4: package storage
why is the package "storage" when the directory is "localstorage" ?

This kind of goes against a lot of our coding standards.

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

https://codereview.appspot.com/13635044/diff/14001/worker/localstorage/worker.go#newcode54
worker/localstorage/worker.go:54: sharedStorageDir :=
os.Getenv(osenv.JujuSharedStorageDir)
I think these have all been moved into the agent config now.

Changes may have been lost during the move from the old directory.

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