agent: add ConfigSetterOnly.SetAPIHostPorts

agent: add ConfigSetterOnly.SetAPIHostPorts

We still only store addresses in the config,
so we lose some metadata. We make use of the
scope metadata before storing, though, to
prune the addresses to the internal ones.

https://code.launchpad.net/~axwalk/juju-core/agent-setapihostports/+merge/213409

(do not edit description out of merge proposal)

[axw, 2014-03-31 06:41:36]

Please take a look.

[dfc, 2014-03-31 06:44:02]

https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go#newcode465
agent/agent_test.go:465: c.Assert(addrs, gc.DeepEquals, []string{"0.1.2.3:123",
"0.1.2.5:125"})
Does conf.APIAddresses return a set or an ordered list ?

[axw, 2014-03-31 07:24:44]

On 2014/03/31 06:44:02, dfc wrote:
> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go
> File agent/agent_test.go (right):
> 
> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go#newcode465
> agent/agent_test.go:465: c.Assert(addrs, gc.DeepEquals,
[]string{"0.1.2.3:123",
> "0.1.2.5:125"})
> Does conf.APIAddresses return a set or an ordered list ?

APIAddresses returns the same order as specified in the list in the config file.
I'm told order is important for addresses, which is why I've done it with
DeepEquals.

[dfc, 2014-03-31 07:27:24]

Fair enough.

On Mon, Mar 31, 2014 at 6:24 PM,  <axwalk@gmail.com> wrote:
> On 2014/03/31 06:44:02, dfc wrote:
>>
>> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go
>> File agent/agent_test.go (right):
>
>
>
> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go#newcode465
>>
>> agent/agent_test.go:465: c.Assert(addrs, gc.DeepEquals,
>
> []string{"0.1.2.3:123",
>>
>> "0.1.2.5:125"})
>> Does conf.APIAddresses return a set or an ordered list ?
>
>
> APIAddresses returns the same order as specified in the list in the
> config file.
> I'm told order is important for addresses, which is why I've done it
> with DeepEquals.
>
> https://codereview.appspot.com/82520043/

[dimitern, 2014-03-31 11:57:11]

LGTM

https://codereview.appspot.com/82520043/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode432
agent/agent.go:432: return
Why return here rather than creating apiDetails?

[jameinel, 2014-03-31 12:26:15]

This feels like an incorrect layer to be doing the validation of addresses.

I won't block it, but I'd like you to consider if the config level should just
take a list, write it down, and read it back again, and instead the callers of
this would be the ones doing the filtering.

https://codereview.appspot.com/82520043/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode435
agent/agent.go:435: for _, serverHostPorts := range servers {
It feels like the wrong layer for *configInternal* to be the thing that decides
that we save CloudLocal only addresses.

Shouldn't this be done in the layer *above* config, and config is just about
"write this stuff to disk and read it back again" ?

[axw, 2014-03-31 13:16:26]

https://codereview.appspot.com/82520043/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode432
agent/agent.go:432: return
On 2014/03/31 11:57:11, dimitern wrote:
> Why return here rather than creating apiDetails?

Because apiDetails should be != nil if we're the API (i.e. always). We should
probably just error out if Addresses is empty in NewAgentConfig.

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode435
agent/agent.go:435: for _, serverHostPorts := range servers {
On 2014/03/31 12:26:15, jameinel wrote:
> It feels like the wrong layer for *configInternal* to be the thing that
decides
> that we save CloudLocal only addresses.
> 
> Shouldn't this be done in the layer *above* config, and config is just about
> "write this stuff to disk and read it back again" ?

You're right, I think, but I think it'd be unhelpful to change that now.
Eventually we should store [][]instance.HostPort in agent.conf, and then use
that in api.Open.