Merge lp:~axwalk/juju-core/lp1212903-debug-hooks-prompt-take2 into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

debug-hooks: include unit/hook in PS1

Set a useful bash prompt, including the
unit and hook names. The tmux window name
gets truncated, so that's not super useful.

Fixes bug #1212903

https://codereview.appspot.com/13044043/

debug-hooks: include unit/hook in PS1

Set a useful bash prompt, including the
unit and hook names. The tmux window name
gets truncated, so that's not super useful.

Fixes bug #1212903

https://codereview.appspot.com/13044043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+180470_code.launchpad.net,

Message:
Please take a look.

Description:
debug-hooks: include unit/hook in PS1

Set a useful bash prompt, including the
unit and hook names. The tmux window name
gets truncated, so that's not super useful.

Fixes bug #1212903

https://code.launchpad.net/~axwalk/juju-core/lp1212903-debug-hooks-prompt-take2/+merge/180470

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M worker/uniter/debug/server.go
   M worker/uniter/debug/server_test.go

[Tim Penhey (thumper)]

LGTM - the code looks fine, did you test it interactively?

https://codereview.appspot.com/13044043/

[Andrew Wilkins (axwalk)]

On 2013/08/16 05:03:44, thumper wrote:
> LGTM - the code looks fine, did you test it interactively?

I did indeed (I probably would've missed the PS1 reset issue otherwise)

https://codereview.appspot.com/13044043/

[John A Meinel (jameinel)]

It looks a bit odd that we expand the ENV vars early. It might not be an
actual problem, but it does seem strange.

Did you fire multiple concurrent hooks to make sure it didn't overwrite
the prompt in the other terminal?

https://codereview.appspot.com/13044043/diff/1/worker/uniter/debug/server_test.go
File worker/uniter/debug/server_test.go (right):

https://codereview.appspot.com/13044043/diff/1/worker/uniter/debug/server_test.go#newcode203
worker/uniter/debug/server_test.go:203: c.Assert(contents, jc.Contains,
fmt.Sprintf(`PS1="%s:%s %% "`, s.ctx.Unit, hookName))
This looks like you are expecting the PS1 line to read
  "unit-0:db-relation-changed %"

I would have thought it would read:

"$JUJU_UNIT_NAME:$JUJU_HOOK_NAME"

Are we expanding the $* stuff early rather than late?

It would seem like this would cause problems when having multiple hooks
firing,
but maybe we rewrite the shell script on every hook?

https://codereview.appspot.com/13044043/

[Andrew Wilkins (axwalk)]

On 2013/08/18 12:44:29, jameinel wrote:
> It looks a bit odd that we expand the ENV vars early. It might not be
an actual
> problem, but it does seem strange.

It doesn't matter. Each time a debug hook session is started, the server
creates a new temporary directory. The env.sh script is written to this
temporary directory and sourced only by the client for that session.

> Did you fire multiple concurrent hooks to make sure it didn't
overwrite the
> prompt in the other terminal?

I didn't, but how would you? Aren't hooks serialised?

https://codereview.appspot.com/13044043/diff/1/worker/uniter/debug/server_test.go
> File worker/uniter/debug/server_test.go (right):

https://codereview.appspot.com/13044043/diff/1/worker/uniter/debug/server_test.go#newcode203
> worker/uniter/debug/server_test.go:203: c.Assert(contents,
jc.Contains,
> fmt.Sprintf(`PS1="%s:%s %% "`, s.ctx.Unit, hookName))
> This looks like you are expecting the PS1 line to read
> "unit-0:db-relation-changed %"

> I would have thought it would read:

> "$JUJU_UNIT_NAME:$JUJU_HOOK_NAME"

> Are we expanding the $* stuff early rather than late?

> It would seem like this would cause problems when having multiple
hooks firing,
> but maybe we rewrite the shell script on every hook?

Correct. The current environment is written to a temporary file for each
session, then that script is sourced by the tmux (debug hooks) client
shell.

https://codereview.appspot.com/13044043/

[William Reade (fwereade)]

LGTM, thanks

https://codereview.appspot.com/13044043/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in worker/uniter/debug/server_test.go

=== modified file 'worker/uniter/debug/server.go'
--- worker/uniter/debug/server.go	2013-07-31 05:37:22 +0000
+++ worker/uniter/debug/server.go	2013-08-20 01:32:40 +0000
@@ -79,17 +79,19 @@
 export JUJU_DEBUG=$(mktemp -d)
 exec > $JUJU_DEBUG/debug.log >&1
 
+# Set a useful prompt.
+export PS1="$JUJU_UNIT_NAME:$JUJU_HOOK_NAME % "
+
 # Save environment variables and export them for sourcing.
 FILTER='^\(LS_COLORS\|LESSOPEN\|LESSCLOSE\|PWD\)='
-env | grep -v $FILTER > $JUJU_DEBUG/env.sh
-sed -i 's/^/export /' $JUJU_DEBUG/env.sh
+export | grep -v $FILTER > $JUJU_DEBUG/env.sh
 
 # Create an internal script which will load the hook environment.
 cat > $JUJU_DEBUG/hook.sh <<END
 #!/bin/bash
 . $JUJU_DEBUG/env.sh
 echo \$\$ > $JUJU_DEBUG/hook.pid
-exec /bin/bash
+exec /bin/bash --noprofile --norc
 END
 chmod +x $JUJU_DEBUG/hook.sh
 

=== modified file 'worker/uniter/debug/server_test.go'
--- worker/uniter/debug/server_test.go	2013-08-19 11:20:02 +0000
+++ worker/uniter/debug/server_test.go	2013-08-20 01:32:40 +0000
@@ -4,6 +4,7 @@
 package debug_test
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -13,7 +14,7 @@
 
 	gc "launchpad.net/gocheck"
 
-	"launchpad.net/juju-core/testing/checkers"
+	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/worker/uniter/debug"
 )
 
@@ -77,7 +78,7 @@
 	session, err = s.ctx.FindSession()
 	c.Assert(session, gc.IsNil)
 	c.Assert(err, gc.NotNil)
-	c.Assert(err, checkers.Satisfies, os.IsNotExist)
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
 
 	// Hooks file is present, empty.
 	err = ioutil.WriteFile(s.ctx.ClientFileLock(), []byte{}, 0777)
@@ -86,8 +87,8 @@
 	c.Assert(session, gc.NotNil)
 	c.Assert(err, gc.IsNil)
 	// If session.hooks is empty, it'll match anything.
-	c.Assert(session.MatchHook(""), gc.Equals, true)
-	c.Assert(session.MatchHook("something"), gc.Equals, true)
+	c.Assert(session.MatchHook(""), jc.IsTrue)
+	c.Assert(session.MatchHook("something"), jc.IsTrue)
 
 	// Hooks file is present, non-empty
 	err = ioutil.WriteFile(s.ctx.ClientFileLock(), []byte(`hooks: [foo, bar, baz]`), 0777)
@@ -96,12 +97,12 @@
 	c.Assert(session, gc.NotNil)
 	c.Assert(err, gc.IsNil)
 	// session should only match "foo", "bar" or "baz".
-	c.Assert(session.MatchHook(""), gc.Equals, false)
-	c.Assert(session.MatchHook("something"), gc.Equals, false)
-	c.Assert(session.MatchHook("foo"), gc.Equals, true)
-	c.Assert(session.MatchHook("bar"), gc.Equals, true)
-	c.Assert(session.MatchHook("baz"), gc.Equals, true)
-	c.Assert(session.MatchHook("foo bar baz"), gc.Equals, false)
+	c.Assert(session.MatchHook(""), jc.IsFalse)
+	c.Assert(session.MatchHook("something"), jc.IsFalse)
+	c.Assert(session.MatchHook("foo"), jc.IsTrue)
+	c.Assert(session.MatchHook("bar"), jc.IsTrue)
+	c.Assert(session.MatchHook("baz"), jc.IsTrue)
+	c.Assert(session.MatchHook("foo bar baz"), jc.IsFalse)
 }
 
 func (s *DebugHooksServerSuite) TestRunHookExceptional(c *gc.C) {
@@ -126,7 +127,7 @@
 	expected := time.Now().Add(time.Second)
 	err = session.RunHook("myhook", s.tmpdir, os.Environ())
 	after := time.Now()
-	c.Assert(after, checkers.TimeBetween(expected.Add(-100*time.Millisecond), expected.Add(100*time.Millisecond)))
+	c.Assert(after, jc.TimeBetween(expected.Add(-100*time.Millisecond), expected.Add(100*time.Millisecond)))
 	c.Assert(err, gc.ErrorMatches, "signal: killed")
 	c.Assert(cmd.Wait(), gc.IsNil)
 }
@@ -138,6 +139,8 @@
 	c.Assert(session, gc.NotNil)
 	c.Assert(err, gc.IsNil)
 
+	const hookName = "myhook"
+
 	// Run the hook in debug mode with the exit flock held,
 	// and also create the .pid file. We'll populate it with
 	// an invalid PID; this will cause the server process to
@@ -146,13 +149,13 @@
 	c.Assert(cmd.Start(), gc.IsNil)
 	ch := make(chan error)
 	go func() {
-		ch <- session.RunHook("myhook", s.tmpdir, os.Environ())
+		ch <- session.RunHook(hookName, s.tmpdir, os.Environ())
 	}()
 	var debugdir os.FileInfo
 	for i := 0; i < 10; i++ {
 		tmpdir, err := os.Open(s.tmpdir)
 		if err != nil {
-			c.Fatalf("Faiked to open $TMPDIR: %s", err)
+			c.Fatalf("Failed to open $TMPDIR: %s", err)
 		}
 		fi, err := tmpdir.Readdir(-1)
 		if err != nil {
@@ -176,6 +179,9 @@
 	if debugdir == nil {
 		c.Error("could not find hook.sh")
 	} else {
+		envsh := filepath.Join(s.tmpdir, debugdir.Name(), "env.sh")
+		s.verifyEnvshFile(c, envsh, hookName)
+
 		hookpid := filepath.Join(s.tmpdir, debugdir.Name(), "hook.pid")
 		err = ioutil.WriteFile(hookpid, []byte("not a pid"), 0777)
 		c.Assert(err, gc.IsNil)
@@ -187,3 +193,12 @@
 	}
 	cmd.Process.Kill() // kill flock
 }
+
+func (s *DebugHooksServerSuite) verifyEnvshFile(c *gc.C, envshPath string, hookName string) {
+	data, err := ioutil.ReadFile(envshPath)
+	c.Assert(err, gc.IsNil)
+	contents := string(data)
+	c.Assert(contents, jc.Contains, fmt.Sprintf("JUJU_UNIT_NAME=%q", s.ctx.Unit))
+	c.Assert(contents, jc.Contains, fmt.Sprintf("JUJU_HOOK_NAME=%q", hookName))
+	c.Assert(contents, jc.Contains, fmt.Sprintf(`PS1="%s:%s %% "`, s.ctx.Unit, hookName))
+}