Looks great in general. Quite a few minor points but nothing too substantial. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode715 state/api/client.go:715: func (c *Client) Version() (version.Number, error) { I think I'd call this AgentVersion. We might have different ideas of API version later. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode720 state/api/client.go:720: if result.Error != nil { 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://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode726 state/api/client.go:726: // Allow overriding in tests. I'd be tempted to name this websocketDialConfig, because that's what we're mocking. // websocketDialConfig is called instead of websocket.DialConfig // so we can override it in tests. ? https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode735 state/api/client.go:735: // IsConnectionError returns true if the error is a connection error. // IsConnectionError reports whether the error is a connection // error. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode741 state/api/client.go:741: // Params for WatchDebugLog Please can we document this type and its fields? This is the public face of the Go Juju API. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode755 state/api/client.go:755: // machines or units. The watching is started the given number of The last sentence doesn't seem to fit here. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode756 state/api/client.go:756: // matching lines back in history. // 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://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode759 state/api/client.go:759: // end point (not sure why). So do a version 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://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode768 state/api/client.go:768: attrs["replay"] = []string{strconv.FormatBool(args.Replay)} attrs.Set(fmt.Sprint(args.Replay)) https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode771 state/api/client.go:771: attrs["maxLines"] = []string{fmt.Sprint(args.Limit)} attrs.Set(fmt.Sprint(args.Limit)) etc https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode780 state/api/client.go:780: if len(arg) > 0 { 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["includeEntity"] = args.IncludeEntity etc should probably work fine. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode804 state/api/client.go:804: // reads too much from the reader. Actually, you can just do: data := make([]byte, maxInitialLineSize) n, err := connection.Read(data) 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://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode820 state/api/client.go:820: logger.Debugf("initial line: %v", string(line)) you can use %s and lose the string conversion here. https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode827 state/api/client.go:827: return nil, fmt.Errorf(errResult.Error.Message) return nil, errResult.Error 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#newcode126 state/api/client_test.go:126: junk := bytes.NewBufferString("junk\n") or strings.NewReader https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode137 state/api/client_test.go:137: junk := bytes.NewBufferString("junk") ditto https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode173 state/api/client_test.go:173: c.Assert(values["includeEntity"], jc.SameContents, params.IncludeEntity) or c.Assert(values, jc.DeepEquals, url.Values{ "includeEntity": params.IncludeEntity, "includeModule": params.IncludeModule, "maxLines": {"100"}, "backlog": {"200"}. etc } https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode185 state/api/client_test.go:185: type closableBuffer struct { You can use ioutil.NopCloser instead of this (if you want the logging, you can just implement a reader that does that only) https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode201 state/api/client_test.go:201: // badReader raises err when read is attempted // badReader returns err when Read is called. ? 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) { 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(¶ms.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 } } https://codereview.appspot.com/85850043/diff/20001/state/apiserver/client/client.go File state/apiserver/client/client.go (right): https://codereview.appspot.com/85850043/diff/20001/state/apiserver/client/client.go#newcode788 state/apiserver/client/client.go:788: return params.VersionResult{Version: ¤t}, nil or just: return params.VersionResult{Version: &version.Current.Number} https://codereview.appspot.com/85850043/diff/20001/utils/http.go File utils/http.go (right): https://codereview.appspot.com/85850043/diff/20001/utils/http.go#newcode96 utils/http.go:96: func CreateBasicAuthHeader(username, password string) 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://codereview.appspot.com/85850043/diff/20001/utils/http_test.go File utils/http_test.go (right): https://codereview.appspot.com/85850043/diff/20001/utils/http_test.go#newcode95 utils/http_test.go:95: fields = strings.Split(string(decoded), ":") or c.Assert(string(decoded), gc.Equals, "eric:sekrit") https://codereview.appspot.com/85850043/