Code review comment for lp:~axwalk/juju-core/lp1223277-check-provisioned-ignore-stderr

Reviewers: mp+184911_code.launchpad.net,

Message:
Please take a look.

Description:
environs/manual: ignore stderr in checkProvisioned

ssh may issue warnings on stderr, for example if
the target host does not exist in known_hosts.
This was tripping up the checkProvisioned function,
which was incorrectly checking that both stdout and
stderr were empty.

Fixes #1223277

https://code.launchpad.net/~axwalk/juju-core/lp1223277-check-provisioned-ignore-stderr/+merge/184911

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13477045/

Affected files (+54, -8 lines):
   A [revision details]
   M environs/manual/detection.go
   M environs/manual/detection_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130910174617-1ib2wgm1qymgkvvs
+New revision: <email address hidden>

Index: environs/manual/detection.go
=== modified file 'environs/manual/detection.go'
--- environs/manual/detection.go 2013-08-23 06:57:09 +0000
+++ environs/manual/detection.go 2013-09-11 01:58:11 +0000
@@ -26,11 +26,16 @@
  func checkProvisioned(sshHost string) (bool, error) {
   cmd := exec.Command("ssh", sshHost, "bash")
   cmd.Stdin = bytes.NewBufferString(checkProvisionedScript)
- out, err := cmd.CombinedOutput()
- if err != nil {
+ var stdout, stderr bytes.Buffer
+ cmd.Stdout = &stdout
+ cmd.Stderr = &stderr
+ if err := cmd.Run(); err != nil {
+ if stderr.Len() != 0 {
+ err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String()))
+ }
    return false, err
   }
- return len(strings.TrimSpace(string(out))) > 0, nil
+ return len(strings.TrimSpace(stdout.String())) > 0, nil
  }

  // detectSeriesHardwareCharacteristics detects the OS series

Index: environs/manual/detection_test.go
=== modified file 'environs/manual/detection_test.go'
--- environs/manual/detection_test.go 2013-08-29 03:58:19 +0000
+++ environs/manual/detection_test.go 2013-09-11 01:58:11 +0000
@@ -13,6 +13,7 @@
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/testing"
+ jc "launchpad.net/juju-core/testing/checkers"
  )

  type detectionSuite struct {
@@ -34,7 +35,10 @@
          echo "ERROR: did not match expected input" >&2
          exit $exitcode
      fi
-%s
+ # stdout
+ %s
+ # stderr
+ %s
      exit %d
  else
      export PATH=${PATH#*:}
@@ -44,14 +48,25 @@
  // sshresponse creates a fake "ssh" command in a new $PATH,
  // updates $PATH, and returns a function to reset $PATH to
  // its original value when called.
-func sshresponse(c *gc.C, input, output string, rc int) func() {
+//
+// output may be:
+// - nil (no output)
+// - a string (stdout)
+// - a slice of strings, of length two (stdout, stderr)
+func sshresponse(c *gc.C, input string, output interface{}, rc int) func()
{
   fakebin := c.MkDir()
   ssh := filepath.Join(fakebin, "ssh")
   sshexpectedinput := ssh + ".expected-input"
- if output != "" {
- output = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
+ var stdout, stderr string
+ switch output := output.(type) {
+ case nil:
+ case string:
+ stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
+ case []string:
+ stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output[0])
+ stderr = fmt.Sprintf("cat>&2<<EOF\n%s\nEOF", output[1])
   }
- script := fmt.Sprintf(sshscript, output, rc)
+ script := fmt.Sprintf(sshscript, stdout, stderr, rc)
   err := ioutil.WriteFile(ssh, []byte(script), 0777)
   c.Assert(err, gc.IsNil)
   err = ioutil.WriteFile(sshexpectedinput, []byte(input), 0644)
@@ -139,3 +154,27 @@
    c.Assert(hc.String(), gc.Equals, test.expectedHc)
   }
  }
+
+func (s *detectionSuite) TestCheckProvisioned(c *gc.C) {
+ defer sshresponse(c, checkProvisionedScript, "", 0)()
+ provisioned, err := checkProvisioned("example.com")
+ c.Assert(err, gc.IsNil)
+ c.Assert(provisioned, jc.IsFalse)
+
+ defer sshresponse(c, checkProvisionedScript, "non-empty", 0)()
+ provisioned, err = checkProvisioned("example.com")
+ c.Assert(err, gc.IsNil)
+ c.Assert(provisioned, jc.IsTrue)
+
+ // stderr should not affect result.
+ defer sshresponse(c, checkProvisionedScript,
[]string{"", "non-empty-stderr"}, 0)()
+ provisioned, err = checkProvisioned("example.com")
+ c.Assert(err, gc.IsNil)
+ c.Assert(provisioned, jc.IsFalse)
+
+ // if the script fails for whatever reason, then checkProvisioned
+ // will return an error. stderr will be included in the error message.
+ defer sshresponse(c, checkProvisionedScript,
[]string{"non-empty-stdout", "non-empty-stderr"}, 255)()
+ _, err = checkProvisioned("example.com")
+ c.Assert(err, gc.ErrorMatches, "exit status 255 \\(non-empty-stderr\\)")
+}