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}
On 2013/09/16 03:02:08, thumper wrote:
> 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.

The storage must be passed down to environs/bootstrap Bootstrap, and
then to environs/tools FindTools. Possibly more places. Those things
also want an Environ (in the case of FindTools, it's hidden in an
interface conversion for the legacy fallback codepath).

Bootstrap storage must be used for *everything* until the environment is
bootstrapped. Leaving a method for acquiring broken/unusable storage
seems like a mistake to me.

> 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.

Fair point, done.

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) {
On 2013/09/14 07:28:03, rog wrote:
> Doc comment please.

Done. Seems someone else came along and wrote a writer constructor in
the mean time. It lacks a doc comment, so I'll add to that :)

> Also, wouldn't it make more sense to define NewFileStorageReader in
terms of
> NewFileStorage? That way you wouldn't need the dynamic type
conversion, as
> Storage can be assigned to StorageReader.

That would simplify the code here. My rationale for doing it this way is
that it limits exposure.

IMO, you shouldn't be able to convert a StorageReader to a Storage,
unless you originally created it with NewStorage and converted it to
StorageReader.

> For future consideration, I think filestorage.New and
filestorage.NewReader
> would be more idiomatic names than filestorage.NewFileStorage and
> filestorage.NewFileStorageReader.

I do agree, but sadly it seems there's no consensus.

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.

It would seem someone else has come along and written a filestorage
writer in the mean time. I'm going to have to touch a bunch more files,
so I'll do this in a followup if that's okay.

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#newcode41
environs/manual/agent.go:41: machineenv map[string]string
On 2013/09/14 07:28:03, rog wrote:
> s/machineenv/machineEnv/

Done.

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{})
On 2013/09/16 03:02:08, thumper wrote:
> 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.

I've renamed it to environConfig.

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
On 2013/09/16 03:02:08, thumper wrote:
> 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).

Copy and paste of my reply to William: I'm not a big fan of the "HasX"
style. IMO, the name should represent the behaviour or role, not
something an implementer has.

Either way, we ought to be consistent. I lean towards being consistent
with "idiomatic Go", where an interface is typically named after an
implementer's behaviour.

I think the variable name you've assigned to in your example is telling.
The name "bootstrapStorage" implies to me that it is itself a Storage.
What else might you call it? Maybe hasBootstrapStorage? That would imply
to me it's a boolean.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode42
environs/manual/bootstrap.go:42: var errHostEmpty = errors.New("Host
argument is empty")
On 2013/09/14 07:28:03, rog wrote:
> Errors don't generally start with a capital letter.

That was meant to relate to the field name, but that was probably a bit
misguided.

> Also, if it's only used once, I wouldn't bother defining a variable
for it.

Fair enough, inlined.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode43
environs/manual/bootstrap.go:43: var errEnvironNil = errors.New("Environ
argument is nil")
On 2013/09/14 07:28:03, rog wrote:
> ditto

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode51
environs/manual/bootstrap.go:51: // host will manually bootstrapped.
On 2013/09/14 07:28:03, rog wrote:
> s/will/will be/ ?

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode65
environs/manual/bootstrap.go:65: err = fmt.Errorf("error checking if
provisioned: %v", err)
On 2013/09/14 07:28:03, rog wrote:
> "failed to check provisioned status: %v" ?

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode78
environs/manual/bootstrap.go:78: defer func() {
On 2013/09/14 07:28:03, rog wrote:
> Why not defer this after the call to SaveState and thereby lose the
need for the
> savedState variable?

Done. Not sure why I did it like that. I may have had something outside
the condition before.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode87
environs/manual/bootstrap.go:87: err = fmt.Errorf("error detecting
hardware characteristics: %v", err)
On 2013/09/14 07:28:03, rog wrote:
> s/err = /return /

Done. I think at one stage it must've spilled over 80 chars.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode123
environs/manual/bootstrap.go:123: machineenv := map[string]string{
On 2013/09/14 07:28:03, rog wrote:
> s/machineenv/machineEnv/

Done.

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode22
environs/manual/detection.go:22: const checkProvisionedScript = "ls
/etc/init/ | grep juju.*\\.conf || exit 0"
On 2013/09/14 07:28:03, rog wrote:
> How about a more direct test?

> "ls /etc/init/juju*.conf"

> which will succeed if provisioned and fail otherwise.

I didn't do that because I wanted to be able to distinguish between an
ssh failure and a command failure.

The alternative is to check error code 255. This requires OS-specific
code for converting to WaitStatus, or whatever the client is running.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode28
environs/manual/detection.go:28: cmd := exec.Command("ssh", sshHost,
"bash")
On 2013/09/14 07:28:03, rog wrote:
> or

> cmd := exec.Command("ssh", sshHost, "bash", "-c",
checkProvisionedScript)

> ?

Indeed. No idea why I didn't do that in the first place.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode30
environs/manual/detection.go:30: out, err := cmd.CombinedOutput()
On 2013/09/14 07:28:03, rog wrote:
> I think you want Output not CombinedOutput here.

Yep, this has been fixed in another MP.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode125
environs/manual/detection.go:125: lsb_release -cs
On 2013/09/14 07:28:03, rog wrote:
> I wonder if this should include a "set -e"

Yes, I think so. Done.

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

https://codereview.appspot.com/13635044/diff/14001/environs/manual/provisioner_test.go#newcode58
environs/manual/provisioner_test.go:58: for i, errorCode := range
[]int{255, 0} {
On 2013/09/14 07:28:03, rog wrote:
> Please add a log statement here, so that if the test fails, we
> know which iteration we were on.

> Something like this is conventional:

> c.Logf("test %d: code %d", i, errorCode)

Done.

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#newcode41
environs/manual/sshresponse_test.go:41: func sshresponse(c *gc.C, input,
output string, rc int) func() {
On 2013/09/14 07:28:03, rog wrote:
> I found these names quite confusing; the respond method doesn't
> respond to anything...

> How about something like "installFakeSSH" instead instead of
sshresponse?

> Then the type could be fakeSSH, and the respond method could be named
SetUp or
> similar.

Done.

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"))
On 2013/09/16 03:02:08, thumper wrote:
> 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")))
> }


That sounds fine. Since there's no way in hell our tests could run in
parallel, we could probably go a step further and make it a top-level
function in juju-core/testing. Then PatchEnvironment itself could be
responsible for calling AddCleanup.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshresponse_test.go#newcode70
environs/manual/sshresponse_test.go:70: func (r sshresponder) respond(c
*gc.C) func() {
On 2013/09/14 07:28:03, rog wrote:
> I'd like to see a doc comment on this please.

Done.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/sshresponse_test.go#newcode87
environs/manual/sshresponse_test.go:87: f := sshresponse(c, input,
output, rc)
On 2013/09/14 07:28:03, rog wrote:
> I think things can be simpler here. How about something like:

> var restores := []func()
> add := func(input, output string, rc int) {
>      restore := sshresponse(c, input, output, rc)
>      restores = append(restores, restore)
> }
> if !r.skipProvisionAgent {
>      add("", "", r.provisionAgentExitCode)
> }
> add(detectionScript, detectionoutput, 0)
> add(checkProvisionedScript, "", 0)
> return func() {
>      for i := len(restores)-1; i >= 0; i-- {
>          restores[i]()
>      }
> }


> alternatively, you could use jc.Restorer throughout, and
> add a method to it:

> // Add returns a Restorer that restores first f1
> // and then f. It is valid to call this on a nil
> // Restorer.
> func (f Restorer) Add(f1 Restorer) Restorer {
>       return func() {
>           f1.Restore()
>           if f != nil {
>               f.Restore()
>           }
>       }
> }

> then you could do:

> var restore jc.Restorer
> add := func(input, output string, rc int) {
>      restore = restore.Add(sshresponse(c, input, output, rc))
> }
> if !r.skipProvisionAgent {
>      add("", "", r.provisionAgentExitCode)
> }
> add(detectionScript, detectionoutput, 0)
> add(checkProvisionedScript, "", 0)
> return restore


> On balance I prefer the latter, because the defer
> calls can be:

> defer sshresponder{...}.respond().Restore()

> which is more obvious to my eyes (I had missed the extra set of () in
the call).

Much nicer, thank you.

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
On 2013/09/16 03:02:08, thumper wrote:
> why is the package "storage" when the directory is "localstorage" ?

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

Yep, that was a mistake.

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)
On 2013/09/16 03:02:08, thumper wrote:
> I think these have all been moved into the agent config now.

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

Yeah I hadn't merged yet. Done now.

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