Make the API address easily accessible by charms

Make the API address easily accessible by charms

The API server addresses are now available to charms
in an environment variable, included in the hooks
context. This allows charms (e.g. Juju GUI) to easily
retrieve the API address to use when connecting to 
the juju WebSocket server.

Also added distro-info-data to the list of apt 
dependencies required for testing.

Pre implementation discussion with Roger.

https://code.launchpad.net/~frankban/juju-core/bug-1161443-api-addresses/+merge/165881

(do not edit description out of merge proposal)

[frankban, 2013-05-27 13:34:16]

Please take a look.

[fwereade, 2013-05-27 20:53:43]

Sorry, but NOT LGTM. API addresses are expected to change, just like state
addresses. They should be exposed via state, just like state addresses (and then
one day exposed via the API, when we switch to using that for the Uniter).

I'm a bit twitchy about JUJU_API_ADDRESSES itself, tbh, but not really enough so
to block the concept -- it clearly *ought* to be a charm config option, I think,
but I can accept that the oo-shiny of having `juju deploy juju-gui` Just Work is
sufficient justification for its existence.

https://codereview.appspot.com/9641044/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/9641044/diff/1/worker/deployer/simple.go#newco...
worker/deployer/simple.go:55: // unit is deployed.
No they don't. They hold the state server addresses an arbitrarily long time in
the past.

https://codereview.appspot.com/9641044/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/9641044/diff/1/worker/uniter/uniter.go#newcode64
worker/uniter/uniter.go:64: apiAddrs []string) *Uniter {
Again, these addresses will be arbitrarily old by the time we create a hook
context. Not OK IMO.

[frankban, 2013-05-29 14:14:49]

Please take a look.

[frankban, 2013-05-29 14:15:35]

This is the new version of the branch including
changes discussed with William:

- state-port and api-port are now environment
  config options;

- as a consequence, port management is moved from
  each environment to environs.FinishMachineConfig;

- state.Addresses() now retrieves the Mongo live 
  servers from the session;

- added state.APIAddresses() returning a slice of 
  API addresses;

- in the hooks context, the state and API addresses
  are retrieved using the addresser.

[fwereade, 2013-05-30 15:44:14]

Nearly there, just a couple of quibbles.

https://codereview.appspot.com/9641044/diff/6001/environs/maas/util_test.go
File environs/maas/util_test.go (right):

https://codereview.appspot.com/9641044/diff/6001/environs/maas/util_test.go#n...
environs/maas/util_test.go:73: MongoPort:   envConfig.StatePort(),
I'd prefer it if we could call this StatePort across the board.

https://codereview.appspot.com/9641044/diff/6001/schema/schema_test.go
File schema/schema_test.go (right):

https://codereview.appspot.com/9641044/diff/6001/schema/schema_test.go#newcod...
schema/schema_test.go:137: c.Assert(err, ErrorMatches, "<path>: expected number,
got nothing")
Can we have a couple of tests for things like non-whole floats, and float values
out of int64 range? I don't mind how harsh you are about rejecting weird things,
given the use case in play, but I'd like to be explicit about expected behaviour
here.

https://codereview.appspot.com/9641044/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/9641044/diff/6001/state/open.go#newcode291
state/open.go:291: panic(err)
Hmm. Why can't we have an error return here?

[frankban, 2013-05-31 09:39:23]

Please take a look.

https://codereview.appspot.com/9641044/diff/6001/environs/maas/util_test.go
File environs/maas/util_test.go (right):

https://codereview.appspot.com/9641044/diff/6001/environs/maas/util_test.go#n...
environs/maas/util_test.go:73: MongoPort:   envConfig.StatePort(),
On 2013/05/30 15:44:14, fwereade wrote:
> I'd prefer it if we could call this StatePort across the board.

Done.

https://codereview.appspot.com/9641044/diff/6001/schema/schema_test.go
File schema/schema_test.go (right):

https://codereview.appspot.com/9641044/diff/6001/schema/schema_test.go#newcod...
schema/schema_test.go:137: c.Assert(err, ErrorMatches, "<path>: expected number,
got nothing")
On 2013/05/30 15:44:14, fwereade wrote:
> Can we have a couple of tests for things like non-whole floats, and float
values
> out of int64 range? I don't mind how harsh you are about rejecting weird
things,
> given the use case in play, but I'd like to be explicit about expected
behaviour
> here.

Done.

https://codereview.appspot.com/9641044/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/9641044/diff/6001/state/open.go#newcode291
state/open.go:291: panic(err)
On 2013/05/30 15:44:14, fwereade wrote:
> Hmm. Why can't we have an error return here?

Fixed. As discussed, I changed the code so that now both Addresses and
APIAddresses also return an error.
Changed the Addresser interface as required.

[fwereade, 2013-06-03 15:18:34]

LGTM apart from test for APIAddresses.

https://codereview.appspot.com/9641044/diff/15001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/9641044/diff/15001/state/open.go#newcode272
state/open.go:272: }
Maybe do this block after the st.Addresses block? Might make for slightly nicer
errors.

https://codereview.appspot.com/9641044/diff/15001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/9641044/diff/15001/state/state_test.go#newcode63
state/state_test.go:63: c.Assert(s.State.CACert(), DeepEquals, info.CACert)
Do we have a test for APIAddresses?

[frankban, 2013-06-03 17:24:57]

Please take a look.

https://codereview.appspot.com/9641044/diff/15001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/9641044/diff/15001/state/open.go#newcode272
state/open.go:272: }
On 2013/06/03 15:18:35, fwereade wrote:
> Maybe do this block after the st.Addresses block? Might make for slightly
nicer
> errors.

Done.

https://codereview.appspot.com/9641044/diff/15001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/9641044/diff/15001/state/state_test.go#newcode63
state/state_test.go:63: c.Assert(s.State.CACert(), DeepEquals, info.CACert)
On 2013/06/03 15:18:35, fwereade wrote:
> Do we have a test for APIAddresses?

Added.

[dimitern, 2013-06-04 14:02:59]

LGTM with one comment.

https://codereview.appspot.com/9641044/diff/24001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/9641044/diff/24001/environs/config/config_test...
environs/config/config_test.go:326: err: `state-port: expected number, got
"illegal"`,
how about adding a test like "state-port": 3.14 for example? (both for state and
api ports)

[frankban, 2013-06-04 15:04:25]

*** Submitted:

Make the API address easily accessible by charms

The API server addresses are now available to charms
in an environment variable, included in the hooks
context. This allows charms (e.g. Juju GUI) to easily
retrieve the API address to use when connecting to 
the juju WebSocket server.

Also added distro-info-data to the list of apt 
dependencies required for testing.

Pre implementation discussion with Roger.

R=fwereade, dimitern
CC=
https://codereview.appspot.com/9641044

https://codereview.appspot.com/9641044/diff/24001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/9641044/diff/24001/environs/config/config_test...
environs/config/config_test.go:326: err: `state-port: expected number, got
"illegal"`,
On 2013/06/04 14:03:00, dimitern wrote:
> how about adding a test like "state-port": 3.14 for example? (both for state
and
> api ports)

As mentioned in the chat, this case is already tested as part of the schema
tests.

[frankban, 2013-06-04 15:05:37]

Hi William and Dimiter,
thank you both for the reviews!