Merge lp:~axwalk/juju-core/null-provider-storage-testfailure into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

provider/null: fix intermittent test failure

The ssh command line options changed, so we
were checking for the wrong command line.
Made it a bit less fragile by doing a negated
grep.

https://codereview.appspot.com/41230043/

provider/null: fix intermittent test failure

The ssh command line options changed, so we
were checking for the wrong command line.
Made it a bit less fragile by doing a negated
grep.

https://codereview.appspot.com/41230043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+198674_code.launchpad.net,

Message:
Please take a look.

Description:
provider/null: fix intermittent test failure

The ssh command line options changed, so we
were checking for the wrong command line.
Made it a bit less fragile by doing a negated
grep.

https://code.launchpad.net/~axwalk/juju-core/null-provider-storage-testfailure/+merge/198674

(do not edit description out of merge proposal)

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

Affected files (+4, -3 lines):
   A [revision details]
   M provider/null/environ_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-20131211131153-tlnqrzdhu609wtsg
+New revision: <email address hidden>

Index: provider/null/environ_test.go
=== modified file 'provider/null/environ_test.go'
--- provider/null/environ_test.go 2013-10-21 21:49:04 +0000
+++ provider/null/environ_test.go 2013-12-12 06:57:36 +0000
@@ -102,13 +102,12 @@
  func (s *environSuite) TestEnvironBootstrapStorager(c *gc.C) {
   var sshScript = `
  #!/bin/bash --norc
-if [ "$*" = "hostname -- bash" ]; then
+if echo "$*" | grep -q -v sudo; then
      # We're executing bash inside ssh. Wait
      # for input to be written before exiting.
      head -n 1 > /dev/null
+ echo JUJU-RC: $RC
  fi
-exec 0<&- # close stdin
-echo JUJU-RC: $RC
  `[1:]
   bin := c.MkDir()
   ssh := filepath.Join(bin, "ssh")

[John A Meinel (jameinel)]

I don't quite understand why sometimes we use "sudo ssh" and sometimes
not.

Otherwise as I understand it, the difference here is that sometimes we
weren't waiting for content to be written (because the args changed),
and this makes it more stable.

Which sounds like a great change, but it seems strange to use the
presence of sudo to detect this.

So this seems LGTM, but I won't say I fully understand the sudo stuff.
(Which makes it a bit hard to maintain the test because of so much
action at a distance.)

https://codereview.appspot.com/41230043/

[Andrew Wilkins (axwalk)]

On 2013/12/15 13:07:03, jameinel wrote:
> I don't quite understand why sometimes we use "sudo ssh" and sometimes
not.

It's restricted to the initial "install" command just to be defensive
against bugs. There's code in there that does rm -fr <something> :)
We could just su down to the original user, which might be more sane,
and reduce round tripping.

> Otherwise as I understand it, the difference here is that sometimes we
weren't
> waiting for content to be written (because the args changed), and this
makes it
> more stable.

That's correct.

> Which sounds like a great change, but it seems strange to use the
presence of
> sudo to detect this.

Yes, it is a bit awkward. That's an artifact of what you've pointed out
below: we're essentially doing white-box testing of a foreign package
here.

> So this seems LGTM, but I won't say I fully understand the sudo stuff.
(Which
> makes it a bit hard to maintain the test because of so much action at
a
> distance.)

It's not great. I'll try to improve it when I have my other tasks under
control.

https://codereview.appspot.com/41230043/

=== modified file 'provider/null/environ_test.go'
--- provider/null/environ_test.go	2013-10-21 21:49:04 +0000
+++ provider/null/environ_test.go	2013-12-12 06:58:50 +0000
@@ -102,13 +102,12 @@
 func (s *environSuite) TestEnvironBootstrapStorager(c *gc.C) {
 	var sshScript = `
 #!/bin/bash --norc
-if [ "$*" = "hostname -- bash" ]; then
+if echo "$*" | grep -q -v sudo; then
     # We're executing bash inside ssh. Wait
     # for input to be written before exiting.
     head -n 1 > /dev/null
+    echo JUJU-RC: $RC
 fi
-exec 0<&- # close stdin
-echo JUJU-RC: $RC
 `[1:]
 	bin := c.MkDir()
 	ssh := filepath.Join(bin, "ssh")