Consistently order changes :-/

Many providers make their modifications in the order that changes comes. In
python3 this causes things to be inconsistently ordered. That mostly works, but
could result in hidenbugs (e.g. Route53Provider's batching could be completely
different based on the order it sees changes.) Sorting changes consistently
is a good thing and it shouldn't hurt situations where providers are already
doing their own ordering. All-in-all more consistent is better and we have to be
explicit with python 3.

octodns/provider/plan.py

@@ -28,7 +28,11 @@ class Plan(object):
                  delete_pcent_threshold=MAX_SAFE_DELETE_PCENT):
         self.existing = existing
         self.desired = desired
-        self.changes = changes
+        # Sort changes to ensure we always have a consistent ordering for
+        # things that make assumptions about that. Many providers will do their
+        # own ordering to ensure things happen in a way that makes sense to
+        # them and/or is as safe as possible.
+        self.changes = sorted(changes)
         self.exists = exists
         self.update_pcent_threshold = update_pcent_threshold
         self.delete_pcent_threshold = delete_pcent_threshold

octodns/record/__init__.py

@@ -25,6 +25,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))
+
 
 class Create(Change):
 

tests/test_octodns_provider_cloudflare.py

@@ -313,7 +313,7 @@ class TestCloudflareProvider(TestCase):
                  'dns_records/fc12ab34cd5611334422ab3322997653'),
             call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/'
                  'dns_records/fc12ab34cd5611334422ab3322997654')
-        ], any_order=True)
+        ])
 
     def test_update_add_swap(self):
         provider = CloudflareProvider('test', 'email', 'token')

tests/test_octodns_provider_digitalocean.py

@@ -176,7 +176,20 @@ class TestDigitalOceanProvider(TestCase):
             call('GET', '/domains/unit.tests/records', {'page': 1}),
             # delete the initial A record
             call('DELETE', '/domains/unit.tests/records/11189877'),
-            # created at least one of the record with expected data
+            # created at least some of the record with expected data
+            call('POST', '/domains/unit.tests/records', data={
+                'data': '1.2.3.4',
+                'name': '@',
+                'ttl': 300, 'type': 'A'}),
+            call('POST', '/domains/unit.tests/records', data={
+                'data': '1.2.3.5',
+                'name': '@',
+                'ttl': 300, 'type': 'A'}),
+            call('POST', '/domains/unit.tests/records', data={
+                'data': 'ca.unit.tests.',
+                'flags': 0, 'name': '@',
+                'tag': 'issue',
+                'ttl': 3600, 'type': 'CAA'}),
             call('POST', '/domains/unit.tests/records', data={
                 'name': '_srv._tcp',
                 'weight': 20,

tests/test_octodns_provider_dnsimple.py

@@ -139,7 +139,32 @@ class TestDnsimpleProvider(TestCase):
         provider._client._request.assert_has_calls([
             # created the domain
             call('POST', '/domains', data={'name': 'unit.tests'}),
-            # created at least one of the record with expected data
+            # created at least some of the record with expected data
+            call('POST', '/zones/unit.tests/records', data={
+                'content': '1.2.3.4',
+                'type': 'A',
+                'name': '',
+                'ttl': 300}),
+            call('POST', '/zones/unit.tests/records', data={
+                'content': '1.2.3.5',
+                'type': 'A',
+                'name': '',
+                'ttl': 300}),
+            call('POST', '/zones/unit.tests/records', data={
+                'content': '0 issue "ca.unit.tests"',
+                'type': 'CAA',
+                'name': '',
+                'ttl': 3600}),
+            call('POST', '/zones/unit.tests/records', data={
+                'content': '1 1 7491973e5f8b39d5327cd4e08bc81b05f7710b49',
+                'type': 'SSHFP',
+                'name': '',
+                'ttl': 3600}),
+            call('POST', '/zones/unit.tests/records', data={
+                'content': '1 1 bf6b6825d2977c511a475bbefb88aad54a92ac73',
+                'type': 'SSHFP',
+                'name': '',
+                'ttl': 3600}),
             call('POST', '/zones/unit.tests/records', data={
                 'content': '20 30 foo-1.unit.tests.',
                 'priority': 10,

tests/test_octodns_provider_dnsmadeeasy.py

@@ -149,7 +149,27 @@ class TestDnsMadeEasyProvider(TestCase):
             call('POST', '/', data={'name': 'unit.tests'}),
             # get all domains to build the cache
             call('GET', '/'),
-            # created at least one of the record with expected data
+            # created at least some of the record with expected data
+            call('POST', '/123123/records', data={
+                'type': 'A',
+                'name': '',
+                'value': '1.2.3.4',
+                'ttl': 300}),
+            call('POST', '/123123/records', data={
+                'type': 'A',
+                'name': '',
+                'value': '1.2.3.5',
+                'ttl': 300}),
+            call('POST', '/123123/records', data={
+                'type': 'ANAME',
+                'name': '',
+                'value': 'aname.unit.tests.',
+                'ttl': 1800}),
+            call('POST', '/123123/records', data={
+                'name': '',
+                'value': 'ca.unit.tests',
+                'issuerCritical': 0, 'caaType': 'issue',
+                'ttl': 3600, 'type': 'CAA'}),
             call('POST', '/123123/records', data={
                 'name': '_srv._tcp',
                 'weight': 20,

tests/test_octodns_provider_ns1.py

@@ -400,9 +400,9 @@ class TestNs1Provider(TestCase):
         self.assertEquals(3, got_n)
         nsone_zone.loadRecord.assert_has_calls([
             call('unit.tests', u'A'),
-            call('geo', u'A'),
             call('delete-me', u'A'),
-        ], any_order=True)
+            call('geo', u'A'),
+        ])
         mock_record.assert_has_calls([
             call.update(answers=[{'answer': [u'1.2.3.4'], 'meta': {}}],
                         filters=[],
@@ -410,6 +410,8 @@ class TestNs1Provider(TestCase):
             call.update(answers=[{u'answer': [u'1.2.3.4'], u'meta': {}}],
                         filters=[],
                         ttl=32),
+            call.delete(),
+            call.delete(),
             call.update(
                 answers=[
                     {u'answer': [u'101.102.103.104'], u'meta': {}},
@@ -427,9 +429,7 @@ class TestNs1Provider(TestCase):
                     {u'filter': u'select_first_n', u'config': {u'N': 1}},
                 ],
                 ttl=34),
-            call.delete(),
-            call.delete()
-        ], any_order=True)
+        ])
 
     def test_escaping(self):
         provider = Ns1Provider('test', 'api-key')

tests/test_octodns_provider_ovh.py

@@ -382,65 +382,64 @@ class TestOvhProvider(TestCase):
                 get_mock.side_effect = [[100], [101], [102], [103]]
                 provider.apply(plan)
                 wanted_calls = [
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'TXT',
-                         subDomain='txt', target=u'TXT text', ttl=1400),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'DKIM',
-                         subDomain='dkim', target=self.valid_dkim_key,
-                         ttl=1300),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'A',
-                         subDomain=u'', target=u'1.2.3.4', ttl=100),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'SRV',
+                    call('/domain/zone/unit.tests/record', fieldType='A',
+                         subDomain='', target='1.2.3.4', ttl=100),
+                    call('/domain/zone/unit.tests/record', fieldType='AAAA',
+                         subDomain='', target='1:1ec:1::1', ttl=200),
+                    call('/domain/zone/unit.tests/record', fieldType='MX',
+                         subDomain='', target='10 mx1.unit.tests.', ttl=400),
+                    call('/domain/zone/unit.tests/record', fieldType='SPF',
+                         subDomain='',
+                         target='v=spf1 include:unit.texts.redirect ~all',
+                         ttl=1000),
+                    call('/domain/zone/unit.tests/record', fieldType='SSHFP',
+                         subDomain='',
+                         target='1 1 bf6b6825d2977c511a475bbefb88aad54a92ac73',
+                         ttl=1100),
+                    call('/domain/zone/unit.tests/record', fieldType='PTR',
+                         subDomain='4', target='unit.tests.', ttl=900),
+                    call('/domain/zone/unit.tests/record', fieldType='SRV',
                          subDomain='_srv._tcp',
-                         target=u'10 20 30 foo-1.unit.tests.', ttl=800),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'SRV',
+                         target='10 20 30 foo-1.unit.tests.', ttl=800),
+                    call('/domain/zone/unit.tests/record', fieldType='SRV',
                          subDomain='_srv._tcp',
-                         target=u'40 50 60 foo-2.unit.tests.', ttl=800),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'PTR',
-                         subDomain='4', target=u'unit.tests.', ttl=900),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'NS',
-                         subDomain='www3', target=u'ns3.unit.tests.', ttl=700),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'NS',
-                         subDomain='www3', target=u'ns4.unit.tests.', ttl=700),
-                    call(u'/domain/zone/unit.tests/record',
-                         fieldType=u'SSHFP', subDomain=u'', ttl=1100,
-                         target=u'1 1 bf6b6825d2977c511a475bbefb88a'
-                                   u'ad54'
-                                   u'a92ac73',
-                         ),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'AAAA',
-                         subDomain=u'', target=u'1:1ec:1::1', ttl=200),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'MX',
-                         subDomain=u'', target=u'10 mx1.unit.tests.', ttl=400),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'CNAME',
-                         subDomain='www2', target=u'unit.tests.', ttl=300),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'SPF',
-                         subDomain=u'', ttl=1000,
-                         target=u'v=spf1 include:unit.texts.'
-                                u'redirect ~all',
-                         ),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'A',
-                         subDomain='sub', target=u'1.2.3.4', ttl=200),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'NAPTR',
-                         subDomain='naptr', ttl=500,
-                         target=u'10 100 "S" "SIP+D2U" "!^.*$!sip:'
-                                u'info@bar'
-                                u'.example.com!" .'
-                         ),
-                    call(u'/domain/zone/unit.tests/refresh')]
-
-                post_mock.assert_has_calls(wanted_calls, any_order=True)
+                         target='40 50 60 foo-2.unit.tests.', ttl=800),
+                    call('/domain/zone/unit.tests/record', fieldType='DKIM',
+                         subDomain='dkim',
+                         target='p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCxLaG'
+                         '16G4SaEcXVdiIxTg7gKSGbHKQLm30CHib1h9FzS9nkcyvQSyQj1r'
+                         'MFyqC//tft3ohx3nvJl+bGCWxdtLYDSmir9PW54e5CTdxEh8MWRk'
+                         'BO3StF6QG/tAh3aTGDmkqhIJGLb87iHvpmVKqURmEUzJPv5KPJfW'
+                         'LofADI+q9lQIDAQAB', ttl=1300),
+                    call('/domain/zone/unit.tests/record', fieldType='NAPTR',
+                         subDomain='naptr',
+                         target='10 100 "S" "SIP+D2U" "!^.*$!sip:info@bar.exam'
+                         'ple.com!" .', ttl=500),
+                    call('/domain/zone/unit.tests/record', fieldType='A',
+                         subDomain='sub', target='1.2.3.4', ttl=200),
+                    call('/domain/zone/unit.tests/record', fieldType='TXT',
+                         subDomain='txt', target='TXT text', ttl=1400),
+                    call('/domain/zone/unit.tests/record', fieldType='CNAME',
+                         subDomain='www2', target='unit.tests.', ttl=300),
+                    call('/domain/zone/unit.tests/record', fieldType='NS',
+                         subDomain='www3', target='ns3.unit.tests.', ttl=700),
+                    call('/domain/zone/unit.tests/record', fieldType='NS',
+                         subDomain='www3', target='ns4.unit.tests.', ttl=700),
+                    call('/domain/zone/unit.tests/refresh')]
+
+                post_mock.assert_has_calls(wanted_calls)
 
                 # Get for delete calls
                 wanted_get_calls = [
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'TXT',
-                         subDomain='txt'),
-                    call(u'/domain/zone/unit.tests/record', fieldType=u'DKIM',
-                         subDomain='dkim'),
                     call(u'/domain/zone/unit.tests/record', fieldType=u'A',
                          subDomain=u''),
+                    call(u'/domain/zone/unit.tests/record', fieldType=u'DKIM',
+                         subDomain='dkim'),
                     call(u'/domain/zone/unit.tests/record', fieldType=u'A',
-                         subDomain='fake')]
-                get_mock.assert_has_calls(wanted_get_calls, any_order=True)
+                         subDomain='fake'),
+                    call(u'/domain/zone/unit.tests/record', fieldType=u'TXT',
+                         subDomain='txt')]
+                get_mock.assert_has_calls(wanted_get_calls)
                 # 4 delete calls for update and delete
                 delete_mock.assert_has_calls(
                     [call(u'/domain/zone/unit.tests/record/100'),

tests/test_octodns_provider_route53.py

@@ -1882,10 +1882,10 @@ class TestRoute53Provider(TestCase):
     @patch('octodns.provider.route53.Route53Provider._really_apply')
     def test_apply_3(self, really_apply_mock, _):
 
-        # with a max of seven modifications, four calls
+        # with a max of seven modifications, three calls
         provider, plan = self._get_test_plan(7)
         provider.apply(plan)
-        self.assertEquals(4, really_apply_mock.call_count)
+        self.assertEquals(3, really_apply_mock.call_count)
 
     @patch('octodns.provider.route53.Route53Provider._load_records')
     @patch('octodns.provider.route53.Route53Provider._really_apply')