state/api: implement APIHostPorts methods

state/api: implement APIHostPorts methods

We also refactor the API client tests a little, factoring
out the API address-related tests into their
own suite alongside the existing EnvironWatcherTests.

Some changes to existing code:

EnvironWatcherTest renamed to EnvironWatcherTests because
plural seems better when it's there for multiple tests.

I don't see a particular use for passing the backing state
separately to NewEnvironWatcherTests, when the backing
state will do for both purposes.

The error field in the return from EnvironConfig is redundant, so
has been removed. Any client should be checking the
error return from the API call anyway, so it should
not have any adverse effect.

api/common/testing moved to api/testing - because
"apitesting" seems like a nice import identifier and it
will be a good place to have other api-related test
functionality if we need it.

https://code.launchpad.net/~rogpeppe/juju-core/526-APIHostPorts-api/+merge/212219

(do not edit description out of merge proposal)

[rog, 2014-03-21 18:48:01]

Please take a look.

[dimitern, 2014-03-24 09:15:59]

LGTM, I like the simplification! Some minor suggestions and questions below:

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
File state/api/common/apiaddresser.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
state/api/common/apiaddresser.go:13: type APIAddresser struct {
doc comment

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
state/api/common/apiaddresser.go:18: // NewAPIAddresser returns
returns... some day? :)

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
state/api/common/apiaddresser.go:40: // CACert returns the certificate used to
validate the state connection.
state only? or both state and API?

https://codereview.appspot.com/78890043/diff/40001/state/api/firewaller/firew...
File state/api/firewaller/firewaller.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/firewaller/firew...
state/api/firewaller/firewaller.go:71: // EnvironConfig returns the current
environment configuration.
Why not change the firewaller API to use common environwatcher as well?

https://codereview.appspot.com/78890043/diff/40001/state/api/provisioner/prov...
File state/api/provisioner/provisioner.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/provisioner/prov...
state/api/provisioner/provisioner.go:24: const provisionerFacade = "Provisioner"
Why using a const only here? I'd pick one - always use a const for a facade name
or never.

https://codereview.appspot.com/78890043/diff/40001/state/api/testing/apiaddre...
File state/api/testing/apiaddresser.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/testing/apiaddre...
state/api/testing/apiaddresser.go:3: package testing
Blank line before package line.

https://codereview.appspot.com/78890043/diff/40001/state/api/uniter/state_tes...
File state/api/uniter/state_test.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:9: gc "launchpad.net/gocheck"
gocheck goes before juju-core imports

https://codereview.appspot.com/78890043/diff/40001/state/api/uniter/uniter.go
File state/api/uniter/uniter.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/uniter/uniter.go...
state/api/uniter/uniter.go:16: const uniterFacade = "Uniter"
here as well - either drop the const or use a const for all other users of
common EnvironWatcher/APIAddresser

https://codereview.appspot.com/78890043/diff/40001/state/apiserver/common/add...
File state/apiserver/common/addresses.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/apiserver/common/add...
state/apiserver/common/addresses.go:62: // TODO(rog) change this to use
api.st.APIHostPorts()
Why not change it now?

[rog, 2014-03-24 10:33:47]

Please take a look.

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
File state/api/common/apiaddresser.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
state/api/common/apiaddresser.go:13: type APIAddresser struct {
On 2014/03/24 09:15:59, dimitern wrote:
> doc comment

Done.

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
state/api/common/apiaddresser.go:18: // NewAPIAddresser returns
On 2014/03/24 09:15:59, dimitern wrote:
> returns... some day? :)

Done.

https://codereview.appspot.com/78890043/diff/40001/state/api/common/apiaddres...
state/api/common/apiaddresser.go:40: // CACert returns the certificate used to
validate the state connection.
On 2014/03/24 09:15:59, dimitern wrote:
> state only? or both state and API?

Done.

https://codereview.appspot.com/78890043/diff/40001/state/api/firewaller/firew...
File state/api/firewaller/firewaller.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/firewaller/firew...
state/api/firewaller/firewaller.go:71: // EnvironConfig returns the current
environment configuration.
On 2014/03/24 09:15:59, dimitern wrote:
> Why not change the firewaller API to use common environwatcher as well?

will do, in a subsequent CL.

https://codereview.appspot.com/78890043/diff/40001/state/api/provisioner/prov...
File state/api/provisioner/provisioner.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/provisioner/prov...
state/api/provisioner/provisioner.go:24: const provisionerFacade = "Provisioner"
On 2014/03/24 09:15:59, dimitern wrote:
> Why using a const only here? I'd pick one - always use a const for a facade
name
> or never.

I'll change the rest of the occurrences in a subsequent CL.

https://codereview.appspot.com/78890043/diff/40001/state/api/testing/apiaddre...
File state/api/testing/apiaddresser.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/testing/apiaddre...
state/api/testing/apiaddresser.go:3: package testing
On 2014/03/24 09:15:59, dimitern wrote:
> Blank line before package line.

Done.

https://codereview.appspot.com/78890043/diff/40001/state/api/uniter/state_tes...
File state/api/uniter/state_test.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:9: gc "launchpad.net/gocheck"
On 2014/03/24 09:15:59, dimitern wrote:
> gocheck goes before juju-core imports

Done.

https://codereview.appspot.com/78890043/diff/40001/state/apiserver/common/add...
File state/apiserver/common/addresses.go (right):

https://codereview.appspot.com/78890043/diff/40001/state/apiserver/common/add...
state/apiserver/common/addresses.go:62: // TODO(rog) change this to use
api.st.APIHostPorts()
On 2014/03/24 09:15:59, dimitern wrote:
> Why not change it now?

Because the API addresses collection used by APIHostPorts is
not yet populated (it needs the peergrouper worker to
be integrated into jujud, which hasn't happened yet)