Code review comment for lp:~gz/juju-core/maas_missing_ip_addresses_1236734

Reviewers: mp+189847_code.launchpad.net,

Message:
Please take a look.

Description:
provider/maas: Handle ip_addresses not existing

Older MAAS versions do not return the ip_addresses field in
their instance json. Update the provider to not treat this
as an error, but instead equivalent to having no further
addresses beyond the seperately exposed DNS name.

https://code.launchpad.net/~gz/juju-core/maas_missing_ip_addresses_1236734/+merge/189847

(do not edit description out of merge proposal)

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

Affected files (+59, -10 lines):
   A [revision details]
   M provider/maas/instance.go
   M provider/maas/instance_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-20131008120312-vr5sryx6voukx4o8
+New revision: <email address hidden>

Index: provider/maas/instance.go
=== modified file 'provider/maas/instance.go'
--- provider/maas/instance.go 2013-09-30 19:40:06 +0000
+++ provider/maas/instance.go 2013-10-08 13:29:41 +0000
@@ -76,7 +76,12 @@

  func (mi *maasInstance) ipAddresses() ([]string, error) {
   // we have to do this the hard way, since maasObject doesn't have this
built-in yet
- objs, err := mi.maasObject.GetMap()["ip_addresses"].GetArray()
+ addressArray := mi.maasObject.GetMap()["ip_addresses"]
+ if addressArray.IsNil() {
+ // Older MAAS versions do not return ip_addresses.
+ return nil, nil
+ }
+ objs, err := addressArray.GetArray()
   if err != nil {
    return nil, err
   }

Index: provider/maas/instance_test.go
=== modified file 'provider/maas/instance_test.go'
--- provider/maas/instance_test.go 2013-09-09 07:02:57 +0000
+++ provider/maas/instance_test.go 2013-10-08 13:29:41 +0000
@@ -9,7 +9,6 @@
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/instance"
- jc "launchpad.net/juju-core/testing/checkers"
  )

  type instanceTest struct {
@@ -88,6 +87,12 @@

   expected := []instance.Address{
    {
+ "DNS name",
+ instance.HostName,
+ "",
+ instance.NetworkPublic,
+ },
+ {
     "1.2.3.4",
     instance.Ipv4Address,
     "",
@@ -99,16 +104,53 @@
     "",
     instance.NetworkUnknown,
    },
- {
- "DNS name",
- instance.HostName,
- "",
- instance.NetworkPublic,
- },
   }

   addr, err := inst.Addresses()

- c.Check(err, gc.IsNil)
- c.Check(addr, jc.SameContents, expected)
+ c.Assert(err, gc.IsNil)
+ c.Check(addr, gc.DeepEquals, expected)
+}
+
+func (s *instanceTest) TestAddressesMissing(c *gc.C) {
+ // Older MAAS versions do not have ip_addresses returned, for these
+ // just the DNS name should be returned without error.
+ jsonValue := `{
+ "hostname": "testing.invalid",
+ "system_id": "system_id"
+ }`
+ obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+ inst := maasInstance{&obj, s.environ}
+
+ addr, err := inst.Addresses()
+ c.Assert(err, gc.IsNil)
+ c.Check(addr, gc.DeepEquals, []instance.Address{
+ {"testing.invalid", instance.HostName, "", instance.NetworkPublic},
+ })
+}
+
+func (s *instanceTest) TestAddressesInvalid(c *gc.C) {
+ jsonValue := `{
+ "hostname": "testing.invalid",
+ "system_id": "system_id",
+ "ip_addresses": "incompatible"
+ }`
+ obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+ inst := maasInstance{&obj, s.environ}
+
+ _, err := inst.Addresses()
+ c.Assert(err, gc.NotNil)
+}
+
+func (s *instanceTest) TestAddressesInvalidContents(c *gc.C) {
+ jsonValue := `{
+ "hostname": "testing.invalid",
+ "system_id": "system_id",
+ "ip_addresses": [42]
+ }`
+ obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+ inst := maasInstance{&obj, s.environ}
+
+ _, err := inst.Addresses()
+ c.Assert(err, gc.NotNil)
  }