cmd/jujuc/server: log hook stdout/stderr output

cmd/jujuc/server: log hook stdout/stderr output

The output stops being logged when the hook exits.

https://code.launchpad.net/~rogpeppe/juju-core/061-log-hook-output/+merge/124179

(do not edit description out of merge proposal)

[rog, 2012-09-13 12:42:57]

Please take a look.

[fwereade, 2012-09-13 13:23:04]

LGTM; I like the tests :)

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:126: // that keeps the pipe open.
Lurking discomfort that if a charm *does* do this it's Doing It Wrong, but this
seems like a sensible approach all the same.

[niemeyer, 2012-09-13 14:38:56]

LGTM

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:127: select {
This logic and the comment can be moved to within logger.stop, so we keep
RunHook itself mostly clean from those concerns.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:170: line, err := br.ReadBytes('\n')
Can we use ReadLine here? It's generally boring, but this is a first-class
case.. we'll benefit from the careful line breakage handling and the overrun
prevention. We won't need some of the logic below as well.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:171: if err != nil {
if err != io.EOF {
    log.Printf("cannot read hook output: %v", err)
}

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.go
File cmd/jujuc/server/context_test.go (right):

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:60: type hookSpec struct {
Nice.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:71: // background holds a string to print in
the background after 0.1s.
s/0.1/0.2/ probably, given the internal change?

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:109: printf("(sleep 0.1; echo %s; sleep 10) &",
spec.background)
s/0.1/0.2/?

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:266: c.Errorf("background process holding up
hook execution")
Nice test!

[rog, 2012-09-13 15:22:38]

Please take a look.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:127: select {
On 2012/09/13 14:38:56, niemeyer wrote:
> This logic and the comment can be moved to within logger.stop, so we keep
> RunHook itself mostly clean from those concerns.

Done.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:170: line, err := br.ReadBytes('\n')
On 2012/09/13 14:38:56, niemeyer wrote:
> Can we use ReadLine here? It's generally boring, but this is a first-class
> case.. we'll benefit from the careful line breakage handling and the overrun
> prevention. We won't need some of the logic below as well.

Done.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:171: if err != nil {
On 2012/09/13 14:38:56, niemeyer wrote:
> if err != io.EOF {
>     log.Printf("cannot read hook output: %v", err)
> }

Done.

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.go
File cmd/jujuc/server/context_test.go (right):

https://codereview.appspot.com/6494132/diff/5/cmd/jujuc/server/context_test.g...
cmd/jujuc/server/context_test.go:71: // background holds a string to print in
the background after 0.1s.
On 2012/09/13 14:38:56, niemeyer wrote:
> s/0.1/0.2/ probably, given the internal change?

Done.

[niemeyer, 2012-09-13 15:29:48]

LGTM, thanks

[rog, 2012-09-13 15:45:07]

*** Submitted:

cmd/jujuc/server: log hook stdout/stderr output

The output stops being logged when the hook exits.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6494132