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/