juju-core

Merge lp:~gz/juju-core/maas_missing_ip_addresses_1236734 into lp:~go-bot/juju-core/trunk

  1. maas_missing_ip_addresses_1236734
  2. Merge into trunk
Proposed by Martin Packman
Status: Merged
Approved by: Martin Packman
Approved revision: no longer in the source branch.
Merged at revision: 1959
Proposed branch: lp:~gz/juju-core/maas_missing_ip_addresses_1236734
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 116 lines (+56/-24)
2 files modified
provider/maas/instance.go (+6/-1)
provider/maas/instance_test.go (+50/-23)
To merge this branch: bzr merge lp:~gz/juju-core/maas_missing_ip_addresses_1236734
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+189847@code.launchpad.net

Commit message

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://codereview.appspot.com/14543043/

R=fwereade, rogpeppe

Description of the change

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://codereview.appspot.com/14543043/

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :
Download full text (3.7 KiB)

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) {
+ jsonV...

Read more...

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with one minor suggestion.

https://codereview.appspot.com/14543043/diff/1/provider/maas/instance_test.go
File provider/maas/instance_test.go (right):

https://codereview.appspot.com/14543043/diff/1/provider/maas/instance_test.go#newcode90
provider/maas/instance_test.go:90: "DNS name",
I'd prefer tagged initialisers for Address here and below.

https://codereview.appspot.com/14543043/

Revision history for this message
William Reade (fwereade) wrote :

LGTM, thanks. Can you test this live?

https://codereview.appspot.com/14543043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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:58:51 +0000
@@ -76,7 +76,12 @@
7676
77func (mi *maasInstance) ipAddresses() ([]string, error) {77func (mi *maasInstance) ipAddresses() ([]string, error) {
78 // we have to do this the hard way, since maasObject doesn't have this built-in yet78 // we have to do this the hard way, since maasObject doesn't have this built-in yet
79 objs, err := mi.maasObject.GetMap()["ip_addresses"].GetArray()79 addressArray := mi.maasObject.GetMap()["ip_addresses"]
80 if addressArray.IsNil() {
81 // Older MAAS versions do not return ip_addresses.
82 return nil, nil
83 }
84 objs, err := addressArray.GetArray()
80 if err != nil {85 if err != nil {
81 return nil, err86 return nil, err
82 }87 }
8388
=== 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:58:51 +0000
@@ -9,7 +9,6 @@
9 gc "launchpad.net/gocheck"9 gc "launchpad.net/gocheck"
1010
11 "launchpad.net/juju-core/instance"11 "launchpad.net/juju-core/instance"
12 jc "launchpad.net/juju-core/testing/checkers"
13)12)
1413
15type instanceTest struct {14type instanceTest struct {
@@ -79,36 +78,64 @@
7978
80func (s *instanceTest) TestAddresses(c *gc.C) {79func (s *instanceTest) TestAddresses(c *gc.C) {
81 jsonValue := `{80 jsonValue := `{
82 "hostname": "DNS name", 81 "hostname": "testing.invalid",
83 "system_id": "system_id", 82 "system_id": "system_id",
84 "ip_addresses": [ "1.2.3.4", "fe80::d806:dbff:fe23:1199" ]83 "ip_addresses": [ "1.2.3.4", "fe80::d806:dbff:fe23:1199" ]
85 }`84 }`
86 obj := s.testMAASObject.TestServer.NewNode(jsonValue)85 obj := s.testMAASObject.TestServer.NewNode(jsonValue)
87 inst := maasInstance{&obj, s.environ}86 inst := maasInstance{&obj, s.environ}
8887
89 expected := []instance.Address{88 expected := []instance.Address{
90 {89 {Value: "testing.invalid", Type: instance.HostName, NetworkScope: instance.NetworkPublic},
91 "1.2.3.4",90 instance.NewAddress("1.2.3.4"),
92 instance.Ipv4Address,91 instance.NewAddress("fe80::d806:dbff:fe23:1199"),
93 "",
94 instance.NetworkUnknown,
95 },
96 {
97 "fe80::d806:dbff:fe23:1199",
98 instance.Ipv6Address,
99 "",
100 instance.NetworkUnknown,
101 },
102 {
103 "DNS name",
104 instance.HostName,
105 "",
106 instance.NetworkPublic,
107 },
108 }92 }
10993
110 addr, err := inst.Addresses()94 addr, err := inst.Addresses()
11195
112 c.Check(err, gc.IsNil)96 c.Assert(err, gc.IsNil)
113 c.Check(addr, jc.SameContents, expected)97 c.Check(addr, gc.DeepEquals, expected)
98}
99
100func (s *instanceTest) TestAddressesMissing(c *gc.C) {
101 // Older MAAS versions do not have ip_addresses returned, for these
102 // just the DNS name should be returned without error.
103 jsonValue := `{
104 "hostname": "testing.invalid",
105 "system_id": "system_id"
106 }`
107 obj := s.testMAASObject.TestServer.NewNode(jsonValue)
108 inst := maasInstance{&obj, s.environ}
109
110 addr, err := inst.Addresses()
111 c.Assert(err, gc.IsNil)
112 c.Check(addr, gc.DeepEquals, []instance.Address{
113 {Value: "testing.invalid", Type: instance.HostName, NetworkScope: instance.NetworkPublic},
114 })
115}
116
117func (s *instanceTest) TestAddressesInvalid(c *gc.C) {
118 jsonValue := `{
119 "hostname": "testing.invalid",
120 "system_id": "system_id",
121 "ip_addresses": "incompatible"
122 }`
123 obj := s.testMAASObject.TestServer.NewNode(jsonValue)
124 inst := maasInstance{&obj, s.environ}
125
126 _, err := inst.Addresses()
127 c.Assert(err, gc.NotNil)
128}
129
130func (s *instanceTest) TestAddressesInvalidContents(c *gc.C) {
131 jsonValue := `{
132 "hostname": "testing.invalid",
133 "system_id": "system_id",
134 "ip_addresses": [42]
135 }`
136 obj := s.testMAASObject.TestServer.NewNode(jsonValue)
137 inst := maasInstance{&obj, s.environ}
138
139 _, err := inst.Addresses()
140 c.Assert(err, gc.NotNil)
114}141}

Subscribers

People subscribed via source and target branches

to status/vote changes: