Thanks, this is looking really nice; I have a number of thoughts below,
of varying actionability (and perhaps sanity). Let me know what you
think.


https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go
File cmd/juju/debughooks.go (right):

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go#newcode79
cmd/juju/debughooks.go:79: return fmt.Errorf("unit %q does not contain
hook %q", unit.Name(), hook)
Would you check whether the python includes handling for, say,
"db-relation-*"? If so we should preserve it.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks.go#newcode109
cmd/juju/debughooks.go:109: args := []string{"-l", "ubuntu", "-t", "-o",
"StrictHostKeyChecking no", "-o", "PasswordAuthentication no", host,
"--"}
This looks awfully familiar. Can a useful set of common args be
extracted?

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go
File cmd/juju/debughooks_test.go (right):

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go#newcode29
cmd/juju/debughooks_test.go:29: stderr string
Local convention would lean somewhat towards:

var debugHooksTests = []struct{
    ...
}{{
     args: []string{"mysql/0"},
     result: regexp.QuoteMeta(...),
}, {
     ...
}}

... ie tightening the indentation slightly and explicitly naming every
non-zero field.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go#newcode43
cmd/juju/debughooks_test.go:43: // "*" is a valid hook name: it means
hook everything.
I'd add these comments as an "info" field, and log them -- I find it
really helps when diagnosing surprising failures.

https://codereview.appspot.com/12019043/diff/3001/cmd/juju/debughooks_test.go#newcode114
cmd/juju/debughooks_test.go:114: c.Logf("testing juju debug-hooks %s",
t.args)
Again by approximate mild convention:

for i, test := range debugHooksTests {
     c.Logf("test %d: %s\n%s", i, test.info, test.args)

...might be neatest. The "testing juju debug-hooks" bit is somewhat
redundant in the light of the test name, but the args are clearly
interesting enough to log..

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#newcode13
worker/uniter/context.go:13:
I'd prefer to call this plain "debug".

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#newcode156
worker/uniter/context.go:156:
It's big and distracting, and not really related to this CL; but, as you
hit packages using log it's nice to switch them over to loggo as you do.
A followup doing so would be much appreciated.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/context.go#newcode158
worker/uniter/context.go:158:
maybe pull this block out into a runCharmHook() method?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go
File worker/uniter/debug/client.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go#newcode12
worker/uniter/debug/client.go:12: func (c *DebugHooksContext)
ClientScript(hooks []string) string {
quibble quibble bikeshed -- how about:

func ClientScript(c *DebugHooksContext, hooks []string) string {

..? It's a little bit jarring to have the functionality of a single type
spread across files, and I think this works just as well as a utility
func.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go#newcode23
worker/uniter/debug/client.go:23: s = strings.Replace(s, "{hook_args}",
strings.Join(hooks, " "), -1)
Hmm... maybe store these with a tiny bit more structure, with a view to
easier compatibility if and when we want to change anything related?
Clients of version X will doubtless at some stage connect to units of
version Y, and a bit of potential for flexibility could save a lot of
headaches if and when any of this stuff needs to change. YAML along the
lines of `hooks: [foo, bar, baz]` would be fairly normal. Thoughts?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go#newcode84
worker/uniter/debug/client.go:84: END
I think readability would be enhanced if you pulled this out into a
tmuxConf const and subbed it in at runtime.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/client.go#newcode95
worker/uniter/debug/client.go:95: exec tmux attach -t {unit_name}
Shouldn't we kill the session here? Otherwise, next time the server runs
a hook, it'll find the session and start creating windows and waiting
for input that'll never come. Right?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common.go
File worker/uniter/debug/common.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common.go#newcode15
worker/uniter/debug/common.go:15: type DebugHooksContext struct {
I think we can drop the stuttering -- debug.HooksContext sounds fine to
me.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/common.go#newcode25
worker/uniter/debug/common.go:25: basename :=
fmt.Sprintf("juju-%s-debug-hooks", state.UnitTag(c.Unit))
imminently names.UnitTag, fwiw

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server.go
File worker/uniter/debug/server.go (right):

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server.go#newcode18
worker/uniter/debug/server.go:18: hooks map[string]bool
utils/set.Strings?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server.go#newcode43
worker/uniter/debug/server.go:43: proc.Kill()
What happens in all these goroutines when we run N hook windows in a row
without exiting the client?

On balance, I think it's good that we don't try to kill the session
here, because staying in debug mode when the client unexpectedly
disconnects is actually pretty cool, if you have some reason to believe
that the client is likely to come back at some point; if so, they're
likely to be pleased that we waited for them.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server.go#newcode95
worker/uniter/debug/server.go:95: tmux new-session -d -s $JUJU_UNIT_NAME
2>&1 | cat > /dev/null || true
Is this still needed? Aren't we now using the existence of this session
as a condition for triggering this script?

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

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_test.go#newcode46
worker/uniter/debug/server_test.go:46: s.setenv("PATH",
s.fakebin+":"+os.Getenv("PATH"))
This also looks awfully familiar; would you have a quick look through
the source for similar constructions and maybe open a bug and tag it
tech-debt if it seems like there are wins to be had?

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_test.go#newcode109
worker/uniter/debug/server_test.go:109: // in exceptional situations.
Doesn't match test name, don't think is needed.

https://codereview.appspot.com/12019043/diff/3001/worker/uniter/debug/server_test.go#newcode147
worker/uniter/debug/server_test.go:147: // exit cleanly (as if the PID
were real and no longer running).
Hmm. Is there any way to fail a hook once we're debugging it? A
`juju-fail` hook command would be handy in a couple of contexts then:
(1) charm authors debugging hooks could fail out and retry with a clean
context if they just did `juju resolved --retry` after `juju-fail`ing
and killing the original window, and (2) we could also grab those
messages and put them in status, and thus make it a useful channel for
authors to communicate charm-specific errors back to users. I'm worried
people would abuse it to spam users with, say, python tracebacks in the
middle of status... but we can make it part of the charm approval
guidelines, I guess?

https://codereview.appspot.com/12019043/