Please take a look.


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) {
On 2014/04/09 10:28:40, rog wrote:
> I think I'd call this AgentVersion.
> We might have different ideas of API version later.

Done.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode720
state/api/client.go:720: if result.Error != nil {
On 2014/04/09 10:28:40, rog wrote:
> 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})

I don't like ignoring it, so I'll change the response type.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode726
state/api/client.go:726: // Allow overriding in tests.
On 2014/04/09 10:28:40, rog wrote:
> 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.
> ?

Done.

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.
On 2014/04/09 10:28:40, rog wrote:
> // IsConnectionError reports whether the error is a connection
> // error.

Done.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode741
state/api/client.go:741: // Params for WatchDebugLog
On 2014/04/09 10:28:40, rog wrote:
> Please can we document this type and its fields?
> This is the public face of the Go Juju API.

Done.

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
On 2014/04/09 10:28:40, rog wrote:
> The last sentence doesn't seem to fit here.

Rewritten

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode756
state/api/client.go:756: // matching lines back in history.
On 2014/04/09 10:28:40, rog wrote:

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

The next branch uses the connection error to determine whether to just
report the error and finish, or to try tailing the debug log file over
ssh like we did before as a fallback for older servers.

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
On 2014/04/09 10:28:40, rog wrote:
> 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.

Ah.. thanks for the explanation. I had guessed something like this but
wasn't entirely sure.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode768
state/api/client.go:768: attrs["replay"] =
[]string{strconv.FormatBool(args.Replay)}
On 2014/04/09 10:28:40, rog wrote:
> attrs.Set(fmt.Sprint(args.Replay))

Done.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode771
state/api/client.go:771: attrs["maxLines"] =
[]string{fmt.Sprint(args.Limit)}
On 2014/04/09 10:28:40, rog wrote:
> attrs.Set(fmt.Sprint(args.Limit))
> etc

Done.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode780
state/api/client.go:780: if len(arg) > 0 {
On 2014/04/09 10:28:40, rog wrote:
> 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.

Yes you are right. I should have checked, but both nil and empty slices
are ignored in the Encode method.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode804
state/api/client.go:804: // reads too much from the reader.
On 2014/04/09 10:28:40, rog wrote:
> 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).

Done. I didn't know about the message boundary thing. Very handy.
Thanks for letting me know.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode820
state/api/client.go:820: logger.Debugf("initial line: %v", string(line))
On 2014/04/09 10:28:40, rog wrote:
> you can use %s and lose the string conversion here.

Ah... nice trick. I keep forgetting these.

https://codereview.appspot.com/85850043/diff/20001/state/api/client.go#newcode827
state/api/client.go:827: return nil, fmt.Errorf(errResult.Error.Message)
On 2014/04/09 10:28:40, rog wrote:
> return nil, errResult.Error

Yeah, I think I did that in another place... might be the next branch.
Also handy.

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")
On 2014/04/09 10:28:40, rog wrote:
> or strings.NewReader

Done.

https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode137
state/api/client_test.go:137: junk := bytes.NewBufferString("junk")
On 2014/04/09 10:28:40, rog wrote:
> ditto

Done.

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)
On 2014/04/09 10:28:40, rog wrote:
> or
> c.Assert(values, jc.DeepEquals, url.Values{
>      "includeEntity": params.IncludeEntity,
>      "includeModule": params.IncludeModule,
>      "maxLines": {"100"},
>      "backlog": {"200"}.
>      etc
> }

I was initially concerned about ordering, however since it is a slice
out, and that ordering actually matters on the query values, I guess we
can just use one deep equals.

https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode185
state/api/client_test.go:185: type closableBuffer struct {
On 2014/04/09 10:28:40, rog wrote:
> You can use ioutil.NopCloser instead of this (if
> you want the logging, you can just implement
> a reader that does that only)

Done.

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
On 2014/04/09 10:28:40, rog wrote:
> // badReader returns err when Read is called.
> ?

Done.

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) {
On 2014/04/09 10:28:40, rog wrote:
> 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
> 	}
> }

That is pretty nice. I take it io.Pipe also does the reading message
boundaries?

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
On 2014/04/09 10:28:40, rog wrote:
> or just:

> return params.VersionResult{Version: &version.Current.Number}

Yeah... I guess this was just my paranoia cutting in.  I just preferred
to take the address of a local rather than the address of a global that
is used in other places on the off chance that the result I'm passing
out was touched or modified by anyone else.

If you really care, I'll change it, but sometimes paranoia is good.

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 {
On 2014/04/09 10:28:40, rog wrote:
> just "BasicAuthHeader" might work well as a name here.

> if you don't like that, then NewBasicAuthHeader would
> probably be more idiomatic than Create.

Went with BasicAuthHeader.

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), ":")
On 2014/04/09 10:28:40, rog wrote:
> or c.Assert(string(decoded), gc.Equals, "eric:sekrit")

Yeah, that was getting a little anal.

https://codereview.appspot.com/85850043/