Looks good. Some minor comments and suggestions below.


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}
nice

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

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) {
Doc comment please.

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.

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

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
s/machineenv/machineEnv/

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#newcode42
environs/manual/bootstrap.go:42: var errHostEmpty = errors.New("Host
argument is empty")
Errors don't generally start with a capital letter.

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

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")
ditto

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode51
environs/manual/bootstrap.go:51: // host will manually bootstrapped.
s/will/will be/ ?

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)
"failed to check provisioned status: %v" ?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode78
environs/manual/bootstrap.go:78: defer func() {
Why not defer this after the call to SaveState and thereby lose the need
for the savedState variable?

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)
s/err = /return /

https://codereview.appspot.com/13635044/diff/14001/environs/manual/bootstrap.go#newcode123
environs/manual/bootstrap.go:123: machineenv := map[string]string{
s/machineenv/machineEnv/

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"
How about a more direct test?

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

which will succeed if provisioned and fail otherwise.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode28
environs/manual/detection.go:28: cmd := exec.Command("ssh", sshHost,
"bash")
or

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

?

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode29
environs/manual/detection.go:29: cmd.Stdin =
bytes.NewBufferString(checkProvisionedScript)
strings.NewReader would seem more appropriate if you continue to use
Stdin.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode30
environs/manual/detection.go:30: out, err := cmd.CombinedOutput()
I think you want Output not CombinedOutput here.

https://codereview.appspot.com/13635044/diff/14001/environs/manual/detection.go#newcode125
environs/manual/detection.go:125: lsb_release -cs
I wonder if this should include a "set -e"

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} {
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)

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() {
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.

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() {
I'd like to see a doc comment on this please.

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

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