state: Use machine addresses from unit

state: Use machine addresses from unit

Make Unit.PublicAddress check for addresses an associated machine
by preference and select an appropriate one. This is the first
step towards removing PublicAddress and PrivateAddress from unit.

https://code.launchpad.net/~gz/juju-core/unit_publicaddress_from_machine/+merge/177983

Requires: https://code.launchpad.net/~gz/juju-core/machine_doc_addresses/+merge/177978

(do not edit description out of merge proposal)

[gz, 2013-08-01 01:49:45]

Please take a look.

[gz, 2013-08-01 13:57:08]

Please take a look.

[gz, 2013-08-01 14:21:32]

Please take a look.

[dimitern, 2013-08-01 14:30:00]

LGTM modulo the below

https://codereview.appspot.com/12218043/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode445
state/unit.go:445: publicaddress := u.doc.PublicAddress
publicAddress please;

Also do we need doc.PublicAddress anymore, now that we're using the machine one?

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode449
state/unit.go:449: // XXX(gz) Or treat as not having an address?
I think just logging it and returning false should be enough here, no need to
panic.

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go#newcode191
state/unit_test.go:191: c.Assert(ok, Equals, true)
How about another test when the unit is not assigned to a machine?

[gz, 2013-08-01 14:40:12]

https://codereview.appspot.com/12218043/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode445
state/unit.go:445: publicaddress := u.doc.PublicAddress
On 2013/08/01 14:30:00, dimitern wrote:
> publicAddress please;

Okay.

> Also do we need doc.PublicAddress anymore, now that we're using the machine
one?

Yes, though potentially with some adjustments. We may need, for backwards compat
reasons, to care about charms manually setting a public address on a unit, in
which case we'd have to pay attention to that. Most of that is for a follow up
branch though.

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode449
state/unit.go:449: // XXX(gz) Or treat as not having an address?
On 2013/08/01 14:30:00, dimitern wrote:
> I think just logging it and returning false should be enough here, no need to
> panic.

Okay.

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go#newcode191
state/unit_test.go:191: c.Assert(ok, Equals, true)
On 2013/08/01 14:30:00, dimitern wrote:
> How about another test when the unit is not assigned to a machine?

The existing test above should cover that case fine I think. As mentioned, there
may be other funny scenarios we need to care about later though.

[gz, 2013-08-01 15:35:10]

Please take a look.

[wallyworld, 2013-08-01 23:21:52]

LGTM. Perhaps with another test like Dimiter suggested.
Plus the err in the log message.

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode454
state/unit.go:454: unitLogger.Errorf("unit %v misses machine id %v", u, id)
please include err in the logged message

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode455
state/unit.go:455: return "", false
did we really want to return "" here? why not u.doc.PublicAddress?

[thumper, 2013-08-02 02:24:17]

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode457
state/unit.go:457: publicAddress = instance.SelectPublicAddress(m.Addresses())
Is there likely to be a case where u.doc.PublicAddress is valid, but
m.Addresses() is empty?  Because if there is, we'd be returning "" here instead
of u.doc.PublicAddress

[dimitern, 2013-08-02 08:36:53]

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode29
state/unit.go:29: var unitLogger = loggo.GetLogger("juju.state.unit")
I think this should be "juju.state" and please move it in state/state.go
perhaps?

[dimitern, 2013-08-02 08:36:53]

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode29
state/unit.go:29: var unitLogger = loggo.GetLogger("juju.state.unit")
I think this should be "juju.state" and please move it in state/state.go
perhaps?