Code review comment for lp:~thumper/juju-core/debug-log-api-client

LGTM with a few trivials.
Thanks!

https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go
File state/api/client_test.go (right):

https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode211
state/api/client_test.go:211: func echoUrl(c *gc.C)
func(*websocket.Config) (io.ReadCloser, error) {
On 2014/04/10 03:59:50, thumper wrote:
> On 2014/04/09 10:28:40, rog wrote:
> > Here's the source of your buffering problem.
> > This fixes it:
> >
> > func echoUrl(c *gc.C) func(*websocket.Config) (io.ReadCloser, error)
{
> > message, err := json.Marshal(&params.ErrorResult{})
> > c.Assert(err, gc.IsNil)
> > return func(config *websocket.Config) (io.ReadCloser, error) {
> > pr, pw := io.Pipe()
> > go func() {
> > fmt.Fprintf(pw, "%s\n", message)
> > fmt.Fprintf(pw, "%s\n", config.Location)
> > }()
> > return pr, nil
> > }
> > }

> That is pretty nice. I take it io.Pipe also does the reading message
boundaries?

yup, io.Pipe does no buffering at all - each write corresponds exactly
to each read.

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode738
state/api/client.go:738: // Params for WatchDebugLog controls the
filtering of the log messages. If the
// DebugLogParams holds parameters for WatchDebugLog that
// control the filtering of the log messages.

?

Thanks a lot for documenting these, BTW. It makes a big difference.

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode747
state/api/client.go:747: // are set all modules are considered
included.
s/ / /

Does the inclusion of a logging module here imply all its sub-modules
too?

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode752
state/api/client.go:752: // ExcludeModule lists logging modules to
exclude from the resposne.
ditto

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode755
state/api/client.go:755: // have been sent, the socket is closed.
// If zero, there is no limit.
?

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode763
state/api/client.go:763: // than the end. If replay is true, backlog is
ignored.
Nice. I started commenting on Backlog to say "but what if I want no
lines of backlog?" - then I saw this.

https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode821
state/api/client.go:821: logger.Debugf("initial line: %s", line)
Just realised - we probably want %q here so that we don't get a blank
line in the log.

https://codereview.appspot.com/85850043/diff/60001/state/api/client_test.go
File state/api/client_test.go (right):

https://codereview.appspot.com/85850043/diff/60001/state/api/client_test.go#newcode140
state/api/client_test.go:140: return &badReader{ioutil.NopCloser(junk),
err}, nil
or ioutil.NopCloser(&badReader{err})

then you can lose the ReadCloser field inside badReader.

https://codereview.appspot.com/85850043/diff/60001/state/api/client_test.go#newcode195
state/api/client_test.go:195: func echoUrl(c *gc.C)
func(*websocket.Config) (io.ReadCloser, error) {
s/echoUrl/echoURL/

(Go convention is to capitalise either all or none of an acronym)

https://codereview.appspot.com/85850043/diff/60001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/85850043/diff/60001/state/api/params/internal.go#newcode563
state/api/params/internal.go:563: // apiserver.
s/apiserver/agent running the API server./
?

https://codereview.appspot.com/85850043/diff/60001/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/85850043/diff/60001/state/apiserver/client/client.go#newcode785
state/apiserver/client/client.go:785: // AgentVersion returns the
current version that the api server is running.
s/api/API/

https://codereview.appspot.com/85850043/