Code review comment for lp:~dave-cheney/juju-core/104-fix-lp-1298703

Reviewers: mp+213195_code.launchpad.net,

Message:
Please take a look.

Description:
Fix lp 1298703

One failure was a simple list ordering bug.

The other two more complicated. NewAddress always returns an address
with the scope of NetworkUnknown. In that case the results from calling
unit.Public/PrivateAddress were essentially random.

It feels like there are some other footguns in this area.

Should NewAddress/NewAddresses create NetworkUnknown Address values ?
What happens if they leak into the state and back to the client? How
many other places are these things leaking into.

I'd like to explore removing NewAddress/NewAddresses in a followup, if
they are used only for testing (axw), then they are doing us a
disservice.

Thanks to Ian and Andrew for their assistance

https://code.launchpad.net/~dave-cheney/juju-core/104-fix-lp-1298703/+merge/213195

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/81720043/

Affected files (+17, -9 lines):
   A [revision details]
   M state/machine_test.go
   M state/unit_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140327203820-qrgltgfsli0ff9ci
+New revision: <email address hidden>

Index: state/machine_test.go
=== modified file 'state/machine_test.go'
--- state/machine_test.go 2014-03-26 11:34:48 +0000
+++ state/machine_test.go 2014-03-28 04:37:31 +0000
@@ -1225,7 +1225,7 @@
   c.Assert(err, gc.IsNil)
   err = machine.Refresh()
   c.Assert(err, gc.IsNil)
- c.Assert(machine.Addresses(), gc.DeepEquals, addresses)
+ c.Assert(machine.Addresses(), jc.SameContents, addresses)
  }

  func (s *MachineSuite) TestSetMachineAddresses(c *gc.C) {

Index: state/unit_test.go
=== modified file 'state/unit_test.go'
--- state/unit_test.go 2014-03-13 07:54:56 +0000
+++ state/unit_test.go 2014-03-28 04:37:31 +0000
@@ -179,10 +179,13 @@
   c.Check(address, gc.Equals, "")
   c.Assert(ok, gc.Equals, false)

- addresses := []instance.Address{
- instance.NewAddress("127.0.0.1"),
- instance.NewAddress("8.8.8.8"),
- }
+ public := instance.NewAddress("8.8.8.8")
+ public.NetworkScope = instance.NetworkPublic
+
+ private := instance.NewAddress("127.0.0.1")
+ private.NetworkScope = instance.NetworkCloudLocal
+
+ addresses := []instance.Address{public, private}
   err = machine.SetAddresses(addresses)
   c.Assert(err, gc.IsNil)

@@ -218,10 +221,13 @@
   c.Check(address, gc.Equals, "")
   c.Assert(ok, gc.Equals, false)

- addresses := []instance.Address{
- instance.NewAddress("127.0.0.1"),
- instance.NewAddress("8.8.8.8"),
- }
+ public := instance.NewAddress("8.8.8.8")
+ public.NetworkScope = instance.NetworkPublic
+
+ private := instance.NewAddress("127.0.0.1")
+ private.NetworkScope = instance.NetworkCloudLocal
+
+ addresses := []instance.Address{public, private}
   err = machine.SetAddresses(addresses)
   c.Assert(err, gc.IsNil)