Existing and desired to _extra_changes, desired used by Route53 to get configed

Fixes an issue where we'd be looking for custom healthcheck config on the
existing record object (from the provider) which would never have a custom
setup. Instead looking at desired lets us find what's actually configured to be
the case

octodns/provider/base.py

@@ -92,7 +92,7 @@ class BaseProvider(BaseSource):
         '''
         return True
 
-    def _extra_changes(self, existing, changes):
+    def _extra_changes(self, existing, desired, changes):
         '''
         An opportunity for providers to add extra changes to the plan that are
         necessary to update ancilary record data or configure the zone. E.g.
@@ -117,7 +117,7 @@ class BaseProvider(BaseSource):
             self.log.info('plan:   filtered out %s changes', before - after)
 
         # allow the provider to add extra changes it needs
-        extra = self._extra_changes(existing, changes)
+        extra = self._extra_changes(existing, desired, changes)
         if extra:
             self.log.info('plan:   extra changes\n  %s', '\n  '
                           .join([str(c) for c in extra]))

octodns/provider/powerdns.py

@@ -259,7 +259,7 @@ class PowerDnsBaseProvider(BaseProvider):
     def _get_nameserver_record(self, existing):
         return None
 
-    def _extra_changes(self, existing, _):
+    def _extra_changes(self, existing, _, __):
         self.log.debug('_extra_changes: zone=%s', existing.name)
 
         ns = self._get_nameserver_record(existing)

octodns/provider/route53.py

@@ -583,18 +583,18 @@ class Route53Provider(BaseProvider):
         self._gc_health_checks(change.existing, [])
         return self._gen_mods('DELETE', existing_records)
 
-    def _extra_changes(self, existing, changes):
-        self.log.debug('_extra_changes: existing=%s', existing.name)
-        zone_id = self._get_zone_id(existing.name)
+    def _extra_changes(self, existing, desired, changes):
+        self.log.debug('_extra_changes: desired=%s', desired.name)
+        zone_id = self._get_zone_id(desired.name)
         if not zone_id:
             # zone doesn't exist so no extras to worry about
             return []
         # we'll skip extra checking for anything we're already going to change
         changed = set([c.record for c in changes])
         # ok, now it's time for the reason we're here, we need to go over all
-        # the existing records
+        # the desired records
         extra = []
-        for record in existing.records:
+        for record in desired.records:
             if record in changed:
                 # already have a change for it, skipping
                 continue
@@ -635,8 +635,8 @@ class Route53Provider(BaseProvider):
                     # no health check id or one that isn't the right version
                     pass
                 # no good, doesn't have the right health check, needs an update
-                self.log.debug('_extra_changes:     health-check caused '
-                               'update')
+                self.log.info('_extra_changes:     health-check caused '
+                              'update')
                 extra.append(Update(record, record))
                 # We don't need to process this record any longer
                 break

tests/test_octodns_provider_base.py

@@ -29,7 +29,7 @@ class HelperProvider(BaseProvider):
         return not self.include_change_callback or \
             self.include_change_callback(change)
 
-    def _extra_changes(self, existing, changes):
+    def _extra_changes(self, existing, desired, changes):
         return self.__extra_changes
 
     def _apply(self, plan):

tests/test_octodns_provider_route53.py

@@ -882,26 +882,26 @@ class TestRoute53Provider(TestCase):
         stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {})
 
         # empty is empty
-        existing = Zone('unit.tests.', [])
-        extra = provider._extra_changes(existing, [])
+        desired = Zone('unit.tests.', [])
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals([], extra)
         stubber.assert_no_pending_responses()
 
         # single record w/o geo is empty
-        existing = Zone('unit.tests.', [])
-        record = Record.new(existing, 'a', {
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, 'a', {
             'ttl': 30,
             'type': 'A',
             'value': '1.2.3.4',
         })
-        existing.add_record(record)
-        extra = provider._extra_changes(existing, [])
+        desired.add_record(record)
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals([], extra)
         stubber.assert_no_pending_responses()
 
         # short-circuit for unknown zone
         other = Zone('other.tests.', [])
-        extra = provider._extra_changes(other, [])
+        extra = provider._extra_changes(None, other, [])
         self.assertEquals([], extra)
         stubber.assert_no_pending_responses()
 
@@ -921,8 +921,8 @@ class TestRoute53Provider(TestCase):
         stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {})
 
         # record with geo and no health check returns change
-        existing = Zone('unit.tests.', [])
-        record = Record.new(existing, 'a', {
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, 'a', {
             'ttl': 30,
             'type': 'A',
             'value': '1.2.3.4',
@@ -930,7 +930,7 @@ class TestRoute53Provider(TestCase):
                 'NA': ['2.2.3.4'],
             }
         })
-        existing.add_record(record)
+        desired.add_record(record)
         list_resource_record_sets_resp = {
             'ResourceRecordSets': [{
                 'Name': 'a.unit.tests.',
@@ -949,7 +949,7 @@ class TestRoute53Provider(TestCase):
         stubber.add_response('list_resource_record_sets',
                              list_resource_record_sets_resp,
                              {'HostedZoneId': 'z42'})
-        extra = provider._extra_changes(existing, [])
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals(1, len(extra))
         stubber.assert_no_pending_responses()
 
@@ -969,8 +969,8 @@ class TestRoute53Provider(TestCase):
         stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {})
 
         # record with geo and no health check returns change
-        existing = Zone('unit.tests.', [])
-        record = Record.new(existing, 'a', {
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, 'a', {
             'ttl': 30,
             'type': 'A',
             'value': '1.2.3.4',
@@ -978,7 +978,7 @@ class TestRoute53Provider(TestCase):
                 'NA': ['2.2.3.4'],
             }
         })
-        existing.add_record(record)
+        desired.add_record(record)
         list_resource_record_sets_resp = {
             'ResourceRecordSets': [{
                 'Name': 'a.unit.tests.',
@@ -1014,12 +1014,12 @@ class TestRoute53Provider(TestCase):
             'MaxItems': '100',
             'Marker': '',
         })
-        extra = provider._extra_changes(existing, [])
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals(1, len(extra))
         stubber.assert_no_pending_responses()
 
         for change in (Create(record), Update(record, record), Delete(record)):
-            extra = provider._extra_changes(existing, [change])
+            extra = provider._extra_changes(None, desired, [change])
             self.assertEquals(0, len(extra))
             stubber.assert_no_pending_responses()
 
@@ -1039,8 +1039,8 @@ class TestRoute53Provider(TestCase):
         stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {})
 
         # record with geo and no health check returns change
-        existing = Zone('unit.tests.', [])
-        record = Record.new(existing, 'a', {
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, 'a', {
             'ttl': 30,
             'type': 'A',
             'value': '1.2.3.4',
@@ -1048,7 +1048,7 @@ class TestRoute53Provider(TestCase):
                 'NA': ['2.2.3.4'],
             }
         })
-        existing.add_record(record)
+        desired.add_record(record)
         list_resource_record_sets_resp = {
             'ResourceRecordSets': [{
                 # other name
@@ -1115,7 +1115,7 @@ class TestRoute53Provider(TestCase):
             'MaxItems': '100',
             'Marker': '',
         })
-        extra = provider._extra_changes(existing, [])
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals(0, len(extra))
         stubber.assert_no_pending_responses()
 
@@ -1123,7 +1123,7 @@ class TestRoute53Provider(TestCase):
         record._octodns['healthcheck'] = {
             'path': '/_ready'
         }
-        extra = provider._extra_changes(existing, [])
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals(1, len(extra))
         stubber.assert_no_pending_responses()
 
@@ -1131,7 +1131,7 @@ class TestRoute53Provider(TestCase):
         record._octodns['healthcheck'] = {
             'host': 'foo.bar.io'
         }
-        extra = provider._extra_changes(existing, [])
+        extra = provider._extra_changes(None, desired, [])
         self.assertEquals(1, len(extra))
         stubber.assert_no_pending_responses()