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\\)")
+}