Merge lp:~axwalk/juju-core/environs-manual-detection-stderr into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2167
Proposed branch: lp:~axwalk/juju-core/environs-manual-detection-stderr
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 55 lines (+22/-7)
2 files modified
environs/manual/detection.go (+7/-5)
environs/manual/detection_test.go (+15/-2)
To merge this branch: bzr merge lp:~axwalk/juju-core/environs-manual-detection-stderr
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+199617@code.launchpad.net

Commit message

environs/manual: ignore stderr in detection

SSH warnings were mucking up series/hw detection.

https://codereview.appspot.com/44200043/

Description of the change

environs/manual: ignore stderr in detection

SSH warnings were mucking up series/hw detection.

https://codereview.appspot.com/44200043/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.1 KiB)

Reviewers: mp+199617_code.launchpad.net,

Message:
Please take a look.

Description:
environs/manual: ignore stderr in detection

SSH warnings were mucking up series/hw detection.

https://code.launchpad.net/~axwalk/juju-core/environs-manual-detection-stderr/+merge/199617

(do not edit description out of merge proposal)

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

Affected files (+24, -7 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-20131219033619-8j3q9tyevvm9j34c
+New revision: <email address hidden>

Index: environs/manual/detection.go
=== modified file 'environs/manual/detection.go'
--- environs/manual/detection.go 2013-12-03 05:20:25 +0000
+++ environs/manual/detection.go 2013-12-19 06:39:26 +0000
@@ -54,15 +54,17 @@
  func DetectSeriesAndHardwareCharacteristics(sshHost string) (hc
instance.HardwareCharacteristics, series string, err error) {
   logger.Infof("Detecting series and characteristics on %s", sshHost)
   cmd := ssh.Command(sshHost, []string{"bash"})
+ var stdout, stderr bytes.Buffer
+ cmd.Stdout = &stdout
+ cmd.Stderr = &stderr
   cmd.Stdin = bytes.NewBufferString(detectionScript)
- out, err := cmd.CombinedOutput()
- if err != nil {
- if len(out) != 0 {
- err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(string(out)))
+ if err := cmd.Run(); err != nil {
+ if stderr.Len() != 0 {
+ err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String()))
    }
    return hc, "", err
   }
- lines := strings.Split(string(out), "\n")
+ lines := strings.Split(stdout.String(), "\n")
   series = strings.TrimSpace(lines[0])

   // Normalise arch.

Index: environs/manual/detection_test.go
=== modified file 'environs/manual/detection_test.go'
--- environs/manual/detection_test.go 2013-11-18 04:53:44 +0000
+++ environs/manual/detection_test.go 2013-12-19 06:39:26 +0000
@@ -32,9 +32,22 @@
  }

  func (s *detectionSuite) TestDetectionError(c *gc.C) {
- defer installFakeSSH(c, detectionScript, "oh noes", 33)()
- _, _, err := DetectSeriesAndHardwareCharacteristics("whatever")
+ scriptResponse := strings.Join([]string{
+ "edgy",
+ "armv4",
+ "MemTotal: 4096 kB",
+ "processor: 0",
+ }, "\n")
+ // if the script fails for whatever reason, then checkProvisioned
+ // will return an error. stderr will be included in the error message.
+ defer installFakeSSH(c, detectionScript, []string{scriptResponse, "oh
noes"}, 33)()
+ hc, _, err := DetectSeriesAndHardwareCharacteristics("hostname")
   c.Assert(err, gc.ErrorMatches, "exit status 33 \\(oh noes\\)")
+ // if the script doesn't fail, stderr is simply ignored.
+ defer installFakeSSH(c, detectionScript,
[]string{scriptResponse, "non-empty-stderr"}, 0)()
+ hc, _, err = DetectSeriesAndHardwareCharacteristics("hostname")
+ c.Assert(err, gc.IsNil)
+ c.Assert(hc.String(), gc.Equals, "arch=arm cpu-cores=1 mem=4M")
  }

  func (s *detectionSuite) TestDetectHardwareCharacteristics(c *gc....

Read more...

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 2013/12/19 06:45:43, axw wrote:
> Please take a look.

LGTM assuming ssh really does use stdout and stderr correctly.

https://codereview.appspot.com/44200043/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2013/12/19 14:51:09, rog wrote:
> On 2013/12/19 06:45:43, axw wrote:
> > Please take a look.

> LGTM assuming ssh really does use stdout and stderr correctly.

Yes, I've confirmed that by changing a key in ~/.ssh/known_hosts.

https://codereview.appspot.com/44200043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (190.8 KiB)

The attempt to merge lp:~axwalk/juju-core/environs-manual-detection-stderr into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.213s
ok launchpad.net/juju-core/agent/tools 0.236s
ok launchpad.net/juju-core/bzr 6.929s
ok launchpad.net/juju-core/cert 3.445s
ok launchpad.net/juju-core/charm 0.630s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.073s
ok launchpad.net/juju-core/cloudinit/sshinit 1.126s
ok launchpad.net/juju-core/cmd 0.207s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 180.767s
ok launchpad.net/juju-core/cmd/jujud 48.742s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.773s
? launchpad.net/juju-core/cmd/plugins/juju-update-bootstrap [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.034s
ok launchpad.net/juju-core/container/factory 0.055s
ok launchpad.net/juju-core/container/kvm 0.297s
ok launchpad.net/juju-core/container/kvm/mock 0.049s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.295s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.349s
ok launchpad.net/juju-core/environs 3.075s
ok launchpad.net/juju-core/environs/bootstrap 4.561s
ok launchpad.net/juju-core/environs/cloudinit 0.803s
ok launchpad.net/juju-core/environs/config 1.067s
ok launchpad.net/juju-core/environs/configstore 0.042s
ok launchpad.net/juju-core/environs/filestorage 0.033s
ok launchpad.net/juju-core/environs/httpstorage 1.056s
ok launchpad.net/juju-core/environs/imagemetadata 0.502s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.053s
ok launchpad.net/juju-core/environs/jujutest 0.258s
ok launchpad.net/juju-core/environs/manual 5.912s
ok launchpad.net/juju-core/environs/simplestreams 0.346s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.049s
ok launchpad.net/juju-core/environs/storage 1.165s
ok launchpad.net/juju-core/environs/sync 30.394s
ok launchpad.net/juju-core/environs/testing 0.217s
ok launchpad.net/juju-core/environs/tools 6.914s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.022s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 16.191s
ok launchpad.net/juju-core/juju/osenv 0.017s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.021s
ok launchpad.net/juju-core/names 0.026s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok ...

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (193.0 KiB)

The attempt to merge lp:~axwalk/juju-core/environs-manual-detection-stderr into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.198s
ok launchpad.net/juju-core/agent/tools 0.233s
ok launchpad.net/juju-core/bzr 7.119s
ok launchpad.net/juju-core/cert 2.936s
ok launchpad.net/juju-core/charm 0.609s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.068s
ok launchpad.net/juju-core/cloudinit/sshinit 1.161s
ok launchpad.net/juju-core/cmd 0.217s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 181.534s
ok launchpad.net/juju-core/cmd/jujud 55.724s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.760s
? launchpad.net/juju-core/cmd/plugins/juju-update-bootstrap [no test files]
ok launchpad.net/juju-core/constraints 0.048s
ok launchpad.net/juju-core/container 0.050s
ok launchpad.net/juju-core/container/factory 0.049s
ok launchpad.net/juju-core/container/kvm 0.268s
ok launchpad.net/juju-core/container/kvm/mock 0.036s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.380s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.256s
ok launchpad.net/juju-core/environs 3.279s
ok launchpad.net/juju-core/environs/bootstrap 4.553s
ok launchpad.net/juju-core/environs/cloudinit 0.807s
ok launchpad.net/juju-core/environs/config 1.113s
ok launchpad.net/juju-core/environs/configstore 0.041s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.959s
ok launchpad.net/juju-core/environs/imagemetadata 0.572s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.054s
ok launchpad.net/juju-core/environs/jujutest 0.226s
ok launchpad.net/juju-core/environs/manual 6.006s
ok launchpad.net/juju-core/environs/simplestreams 0.418s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.305s
ok launchpad.net/juju-core/environs/storage 1.268s
ok launchpad.net/juju-core/environs/sync 30.650s
ok launchpad.net/juju-core/environs/testing 0.214s
ok launchpad.net/juju-core/environs/tools 6.734s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.030s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 16.290s
ok launchpad.net/juju-core/juju/osenv 0.018s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.023s
ok launchpad.net/juju-core/names 0.032s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok ...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/manual/detection.go'
2--- environs/manual/detection.go 2013-12-03 05:20:25 +0000
3+++ environs/manual/detection.go 2013-12-19 06:44:00 +0000
4@@ -54,15 +54,17 @@
5 func DetectSeriesAndHardwareCharacteristics(sshHost string) (hc instance.HardwareCharacteristics, series string, err error) {
6 logger.Infof("Detecting series and characteristics on %s", sshHost)
7 cmd := ssh.Command(sshHost, []string{"bash"})
8+ var stdout, stderr bytes.Buffer
9+ cmd.Stdout = &stdout
10+ cmd.Stderr = &stderr
11 cmd.Stdin = bytes.NewBufferString(detectionScript)
12- out, err := cmd.CombinedOutput()
13- if err != nil {
14- if len(out) != 0 {
15- err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(string(out)))
16+ if err := cmd.Run(); err != nil {
17+ if stderr.Len() != 0 {
18+ err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String()))
19 }
20 return hc, "", err
21 }
22- lines := strings.Split(string(out), "\n")
23+ lines := strings.Split(stdout.String(), "\n")
24 series = strings.TrimSpace(lines[0])
25
26 // Normalise arch.
27
28=== modified file 'environs/manual/detection_test.go'
29--- environs/manual/detection_test.go 2013-11-18 04:53:44 +0000
30+++ environs/manual/detection_test.go 2013-12-19 06:44:00 +0000
31@@ -32,9 +32,22 @@
32 }
33
34 func (s *detectionSuite) TestDetectionError(c *gc.C) {
35- defer installFakeSSH(c, detectionScript, "oh noes", 33)()
36- _, _, err := DetectSeriesAndHardwareCharacteristics("whatever")
37+ scriptResponse := strings.Join([]string{
38+ "edgy",
39+ "armv4",
40+ "MemTotal: 4096 kB",
41+ "processor: 0",
42+ }, "\n")
43+ // if the script fails for whatever reason, then checkProvisioned
44+ // will return an error. stderr will be included in the error message.
45+ defer installFakeSSH(c, detectionScript, []string{scriptResponse, "oh noes"}, 33)()
46+ hc, _, err := DetectSeriesAndHardwareCharacteristics("hostname")
47 c.Assert(err, gc.ErrorMatches, "exit status 33 \\(oh noes\\)")
48+ // if the script doesn't fail, stderr is simply ignored.
49+ defer installFakeSSH(c, detectionScript, []string{scriptResponse, "non-empty-stderr"}, 0)()
50+ hc, _, err = DetectSeriesAndHardwareCharacteristics("hostname")
51+ c.Assert(err, gc.IsNil)
52+ c.Assert(hc.String(), gc.Equals, "arch=arm cpu-cores=1 mem=4M")
53 }
54
55 func (s *detectionSuite) TestDetectHardwareCharacteristics(c *gc.C) {

Subscribers

People subscribed via source and target branches

to status/vote changes: