Code review comment for lp:~thumper/juju-core/debug-log-api-client
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Looks great in general. Quite a few minor points but nothing too
substantial.
https:/
File state/api/client.go (right):
https:/
state/api/
error) {
I think I'd call this AgentVersion.
We might have different ideas of API version later.
https:/
state/api/
This isn't possible. The Error field is only there for bulk version
results.
I'd either ignore the field, or make a new type in params
(e.g. type AgentVersionResult {Version version.Number})
https:/
state/api/
I'd be tempted to name this websocketDialCo
we're
mocking.
// websocketDialConfig is called instead of websocket.
// so we can override it in tests.
?
https:/
state/api/
is a connection error.
// IsConnectionError reports whether the error is a connection
// error.
https:/
state/api/
Please can we document this type and its fields?
This is the public face of the Go Juju API.
https:/
state/api/
the given number of
The last sentence doesn't seem to fit here.
https:/
state/api/
// It returns an error that satisfies IsConnectionError
// when the connection cannot be made.
?
I'm not quite sure why we want to distinguish a connection
error from the other kinds of errors though.
https:/
state/api/
check, as version was added
The reason that the websocket connection hangs is that
the server is serving the API on every URL (that was
a mistake that I should have fixed earlier, sorry),
so you're waiting for an RPC reply without sending
any request.
https:/
state/api/
[]string{
attrs.Set(
https:/
state/api/
[]string{
attrs.Set(
etc
https:/
state/api/
Is this actually necessary. If there's a zero-length slice in the
attributes, does it matter if it's nil or empty?
I think just
attrs["
etc
should probably work fine.
https:/
state/api/
Actually, you can just do:
data := make([]byte, maxInitialLineSize)
n, err := connection.
if err != nil {
return nil, fmt.Errorf("unable to read initial response: %v", err)
}
data = data[0:n]
because websockets guarantees that it will read at
most one frame. Alternatively, you could just use
bufio as you tried originally.
The problem you were having with bufio reading too much
data was a problem with your test (your echoUrl function
was returning a bytes.Buffer, not a reader that preserved
message boundaries).
https:/
state/api/
you can use %s and lose the string conversion here.
https:/
state/api/
return nil, errResult.Error
https:/
File state/api/
https:/
state/api/
or strings.NewReader
https:/
state/api/
ditto
https:/
state/api/
jc.SameContents, params.
or
c.Assert(values, jc.DeepEquals, url.Values{
"includeEn
"includeMo
"maxLines": {"100"},
"backlog": {"200"}.
etc
}
https:/
state/api/
You can use ioutil.NopCloser instead of this (if
you want the logging, you can just implement
a reader that does that only)
https:/
state/api/
attempted
// badReader returns err when Read is called.
?
https:/
state/api/
func(*websocket
Here's the source of your buffering problem.
This fixes it:
func echoUrl(c *gc.C) func(*websocket
message, err := json.Marshal(
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
}
}
https:/
File state/apiserver
https:/
state/apiserver
params.
or just:
return params.
https:/
File utils/http.go (right):
https:/
utils/http.go:96: func CreateBasicAuth
http.Header {
just "BasicAuthHeader" might work well as a name here.
if you don't like that, then NewBasicAuthHeader would
probably be more idiomatic than Create.
https:/
File utils/http_test.go (right):
https:/
utils/http_
or c.Assert(
https:/