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

Please take a look.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go
File state/apiserver/debuglog.go (right):

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode39
state/apiserver/debuglog.go:39: // includeAgent -> []string - lists
agent tagsto include in the response
On 2014/04/07 02:21:10, axw wrote:
> s/tagsto/tags to/

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode39
state/apiserver/debuglog.go:39: // includeAgent -> []string - lists
agent tagsto include in the response
On 2014/04/07 02:21:10, axw wrote:
> I think "agent" might be the wrong term here. The user cares about the
entity,
> not the agent responsible for it. Entity is the name we use in state
and API
> when passing tags around like this.

OK. Will change, although I imagine issues later... but we can deal with
that later.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode40
state/apiserver/debuglog.go:40: // may finish with a '*' to match a
prefix eg: unit-mysql-*, machine-2
On 2014/04/07 02:21:10, axw wrote:
> "tags may finish..."
> (would be nice to also begin the line with a hyphen like in the
following line,
> so it's clear that it's not part of the previous sentence)

Done.

> and while I'm being pedantic,
> s/eg/e.g./

Done

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode47
state/apiserver/debuglog.go:47: // limit -> int - show this many lines
then exit
On 2014/04/07 02:21:10, axw wrote:
> show *at most* this many lines

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode49
state/apiserver/debuglog.go:49: // replay -> string - one of [true,
false], if true, start the file from the start
On 2014/04/07 02:21:10, axw wrote:
> replay does not suggest going back to the beginning to me. how about
rewind?

I kept 'replay' because it was what pyjuju used.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode59
state/apiserver/debuglog.go:59: stream, err :=
newLogStream(req.URL.Query())
On 2014/04/07 02:21:10, axw wrote:
> Not a fan of constructor + init, it's too easy to overlook the need to
> initialise later. Is there a good reason not to combine init() into
> newLogStream?

Probably a bad use of 'init'. Will change to "start".

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode104
state/apiserver/debuglog.go:104: return nil, fmt.Errorf("maxLines value
%q is not a valid unsigned number", value)
On 2014/04/07 02:21:10, axw wrote:
> add the error here too?

I had wanted to provide a more useful and specific error to the caller.
But I'll add it.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode113
state/apiserver/debuglog.go:113: return nil, fmt.Errorf("replay value %q
is not a valid boolean", value)
On 2014/04/07 02:21:10, axw wrote:
> ditto

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode122
state/apiserver/debuglog.go:122: return nil, fmt.Errorf("backlog value
%q is not a valid unsigned number", value)
On 2014/04/07 02:21:10, axw wrote:
> ditto

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode166
state/apiserver/debuglog.go:166: // authenticate parses HTTP basic
authentication and authorizes the
On 2014/04/07 02:21:10, axw wrote:
> This is a bit out of place, and appears to be a copy-and-paste from
> apiserver/httphandler.go
> Can you please factor them?

Yep. Didn't notice this was a copy and paste thing...

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode218
state/apiserver/debuglog.go:218: if strings.HasSuffix(agent, ":") {
On 2014/04/07 02:21:10, axw wrote:
> When does it not have the suffix?

Then it is unexpected, and the agent field in the logLine is not set.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_internal_test.go
File state/apiserver/debuglog_internal_test.go (right):

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_internal_test.go#newcode272
state/apiserver/debuglog_internal_test.go:272: expected := `line 1
On 2014/04/07 03:36:04, axw wrote:
> How about a test that has maxLines>available?

Good call. Caught an issue.

https://codereview.appspot.com/84880044/