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(&params.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: &current}, 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/