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

On 2014/04/07 05:02:09, thumper wrote:
> 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.

LGTM, thanks

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