Merge pull request #882 from octodns/change-ordering

Order changes: deletes, creates, updates

CHANGELOG.md

@@ -1,3 +1,19 @@
+## v0.9.17 - 2022-??-?? - ???
+
+#### Noteworthy changes
+
+* The changes in plans are now ordered based on change type prior to
+  considering the record name and type as was previously done. The chosen
+  order is: deletes, creates, updates. The reason for that many providers make
+  changes one at a time. When changing the type of a record, e.g. from A to
+  CNAME of vice versa this is done by deleting the old and creating the new.
+  If the CNAME create happens before the A delete it will often violate
+  rules against having typed records live at the same node as a CNAME. Several
+  providers have always handled this by sorting the changes themselves. This
+  just standardizes what they are doing as many other providers appear to need
+  to do so, but weren't. There was an ordering before, but it was essentially
+  arbitrarily picked.
+
 ## v0.9.16 - 2022-03-04 - Manage the root of the problem
 
 #### Noteworthy changes

octodns/record/__init__.py

@@ -15,7 +15,7 @@ from ..equality import EqualityTupleMixin
 from .geo import GeoCodes
 
 
-class Change(object):
+class Change(EqualityTupleMixin):
 
     def __init__(self, existing, new):
         self.existing = existing
@@ -26,14 +26,12 @@ class Change(object):
         'Returns new if we have one, existing otherwise'
         return self.new or self.existing
 
-    def __lt__(self, other):
-        self_record = self.record
-        other_record = other.record
-        return ((self_record.name, self_record._type) <
-                (other_record.name, other_record._type))
+    def _equality_tuple(self):
+        return (self.CLASS_ORDERING, self.record.name, self.record._type)
 
 
 class Create(Change):
+    CLASS_ORDERING = 1
 
     def __init__(self, new):
         super(Create, self).__init__(None, new)
@@ -44,6 +42,7 @@ class Create(Change):
 
 
 class Update(Change):
+    CLASS_ORDERING = 2
 
     # Leader is just to allow us to work around heven eating leading whitespace
     # in our output. When we call this from the Manager.sync plan summary
@@ -56,6 +55,7 @@ class Update(Change):
 
 
 class Delete(Change):
+    CLASS_ORDERING = 0
 
     def __init__(self, existing):
         super(Delete, self).__init__(existing, None)

tests/test_octodns_plan.py

@@ -54,6 +54,31 @@ plans = [
 ]
 
 
+class TestPlanSortsChanges(TestCase):
+
+    def test_plan_sorts_changes_pass_to_it(self):
+        # we aren't worried about the details of the sorting, that's tested in
+        # test_octodns_record's TestChanges. We just want to make sure that the
+        # changes are sorted at all.
+        zone = Zone('unit.tests.', [])
+        record_a_1 = Record.new(zone, '1', {
+            'type': 'A',
+            'ttl': 30,
+            'value': '1.2.3.4',
+        })
+        create_a_1 = Create(record_a_1)
+        record_a_2 = Record.new(zone, '2', {
+            'type': 'A',
+            'ttl': 30,
+            'value': '1.2.3.4',
+        })
+        create_a_2 = Create(record_a_2)
+
+        # passed in reverse of expected order
+        plan = Plan(None, None, [create_a_2, create_a_1], False)
+        self.assertEqual([create_a_1, create_a_2], plan.changes)
+
+
 class TestPlanLogger(TestCase):
 
     def test_invalid_level(self):

tests/test_octodns_record.py

@@ -5021,3 +5021,76 @@ class TestDynamicRecords(TestCase):
         self.assertEqual(dynamic, dynamic)
         self.assertNotEqual(dynamic, other)
         self.assertNotEqual(dynamic, 42)
+
+
+class TestChanges(TestCase):
+    zone = Zone('unit.tests.', [])
+    record_a_1 = Record.new(zone, '1', {
+        'type': 'A',
+        'ttl': 30,
+        'value': '1.2.3.4',
+    })
+    record_a_2 = Record.new(zone, '2', {
+        'type': 'A',
+        'ttl': 30,
+        'value': '1.2.3.4',
+    })
+    record_aaaa_1 = Record.new(zone, '1', {
+        'type': 'AAAA',
+        'ttl': 30,
+        'value': '2601:644:500:e210:62f8:1dff:feb8:947a',
+    })
+    record_aaaa_2 = Record.new(zone, '2', {
+        'type': 'AAAA',
+        'ttl': 30,
+        'value': '2601:644:500:e210:62f8:1dff:feb8:947a',
+    })
+
+    def test_sort_same_change_type(self):
+        # expect things to be ordered by name and type since all the change
+        # types are the same it doesn't matter
+        changes = [
+            Create(self.record_aaaa_1),
+            Create(self.record_a_2),
+            Create(self.record_a_1),
+            Create(self.record_aaaa_2),
+        ]
+        self.assertEqual([
+            Create(self.record_a_1),
+            Create(self.record_aaaa_1),
+            Create(self.record_a_2),
+            Create(self.record_aaaa_2),
+        ], sorted(changes))
+
+    def test_sort_same_different_type(self):
+        # this time the change type is the deciding factor, deletes come before
+        # creates, and then updates. Things of the same type, go down the line
+        # and sort by name, and then type
+        changes = [
+            Delete(self.record_aaaa_1),
+            Create(self.record_aaaa_1),
+            Update(self.record_aaaa_1, self.record_aaaa_1),
+            Update(self.record_a_1, self.record_a_1),
+            Create(self.record_a_1),
+            Delete(self.record_a_1),
+            Delete(self.record_aaaa_2),
+            Create(self.record_aaaa_2),
+            Update(self.record_aaaa_2, self.record_aaaa_2),
+            Update(self.record_a_2, self.record_a_2),
+            Create(self.record_a_2),
+            Delete(self.record_a_2),
+        ]
+        self.assertEqual([
+            Delete(self.record_a_1),
+            Delete(self.record_aaaa_1),
+            Delete(self.record_a_2),
+            Delete(self.record_aaaa_2),
+            Create(self.record_a_1),
+            Create(self.record_aaaa_1),
+            Create(self.record_a_2),
+            Create(self.record_aaaa_2),
+            Update(self.record_a_1, self.record_a_1),
+            Update(self.record_aaaa_1, self.record_aaaa_1),
+            Update(self.record_a_2, self.record_a_2),
+            Update(self.record_aaaa_2, self.record_aaaa_2),
+        ], sorted(changes))