Code review comment for lp:~axwalk/juju-core/client-cache-apihostports

LGTM with the juju tests fixed so they don't rely on calling methods on
the zero value, and a few other suggestions below.

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go#newcode160
juju/apiconn_test.go:160: // be updated (to an empty slice).
As discussed online, I'm not sure that this is the correct approach - we
shouldn't rely on calling methods on types that we've created with New.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode212
state/api/apiclient.go:212: func (s *State) HostPorts()
[][]instance.HostPort {
On further reflection, I think this method should be named APIHostPorts,
mirroring the same function in State, and making it hopefully clearer
what the returned addresses are about.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode213
state/api/apiclient.go:213: return append([][]instance.HostPort{},
s.hostPorts...)
If you're serious about returning a copy, then you should probably copy
all the sub-slices too.

Otherwise, just return s.hostPorts and document that the returned slice
should not be modified.

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go#newcode40
state/api/state.go:40: if err := st.addAddrHostPort(); err != nil {
rather than have addAddrHostPort be a method that mutates st.hostPorts,
perhaps it might be clearer to have something like:

hostPorts, err := addAddress(result.Servers, st.addr)
if err != nil {
     st.Close()
     return err
}
st.hostPorts = hostPort

// addAddress appends a new server derived from the given
// address to servers if the address is not already found
// there.
func addAddress(servers [][]instance.HostPort, addr string)
([][]instance.HostPort, error)

Then addAddress is easily testable if you want to.

BTW I have mixed feelings about returning an error if we can't split the
dial address. It means that if the dial address isn't parseable for some
reason (e.g. the port is non-numeric) we'll fail the connection, even
though we actually managed to connect.

https://codereview.appspot.com/81780043/