From cdcaef8f2bbc1a1e47b84a61eb9935308e801218 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 4 Mar 2022 14:18:23 -0800 Subject: [PATCH] Order changes: deletes, creates, updates --- CHANGELOG.md | 16 ++++++++ octodns/record/__init__.py | 12 +++--- tests/test_octodns_plan.py | 25 ++++++++++++ tests/test_octodns_record.py | 73 ++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index add77cf..6fdadca 100644 --- a/CHANGELOG.md +++ b/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-??-?? - ??? #### Noteworthy changes diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index 74ac48d..61dfb04 100644 --- a/octodns/record/__init__.py +++ b/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) diff --git a/tests/test_octodns_plan.py b/tests/test_octodns_plan.py index 6c01bb8..d302ceb 100644 --- a/tests/test_octodns_plan.py +++ b/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): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 176c11d..f8819a6 100644 --- a/tests/test_octodns_record.py +++ b/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))