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

Haven't been through the tests yet, but here's my comments so far.

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
s/tagsto/tags to/

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
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.

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
"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)

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

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
show *at most* this many lines

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
replay does not suggest going back to the beginning to me. how about
rewind?

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode59
state/apiserver/debuglog.go:59: stream, err :=
newLogStream(req.URL.Query())
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?

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)
add the error here too?

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)
ditto

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)
ditto

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
This is a bit out of place, and appears to be a copy-and-paste from
apiserver/httphandler.go
Can you please factor them?

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#newcode218
state/apiserver/debuglog.go:218: if strings.HasSuffix(agent, ":") {
When does it not have the suffix?

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