Route53Provider python 3, rm incf.countryutils, lots of cmp removal and ordering fixes

CHANGELOG.md

@@ -1,3 +1,9 @@
+## v0.9.9 - 2019-??-?? - Python 3.7 Support
+
+* Route53 _mod_keyer ordering wasn't complete/reliable and in python 3 this
+  resulted in randomness. This has been addressed and may result in value
+  reordering on next plan, no actual changes in behavior should occur.
+
 ## v0.9.8 - 2019-09-30 - One with no changes b/c PyPi description problems
 
 * No material changes

octodns/provider/route53.py

@@ -8,8 +8,8 @@ from __future__ import absolute_import, division, print_function, \
 from boto3 import client
 from botocore.config import Config
 from collections import defaultdict
-from incf.countryutils.transformations import cca_to_ctca2
 from ipaddress import AddressValueError, ip_address
+from pycountry_convert import country_alpha2_to_continent_code
 from uuid import uuid4
 import logging
 import re
@@ -20,13 +20,6 @@ from ..record import Record, Update
 from ..record.geo import GeoCodes
 from .base import BaseProvider
 
-# TODO: remove when Python 2.x is no longer supported
-try:  # pragma: no cover
-    cmp
-except NameError:  # pragma: no cover
-    def cmp(x, y):
-        return (x > y) - (x < y)
-
 octal_re = re.compile(r'\\(\d\d\d)')
 
 
@@ -155,7 +148,7 @@ class _Route53Record(object):
             }
         }
 
-    # NOTE: we're using __hash__ and __cmp__ methods that consider
+    # NOTE: we're using __hash__ and ordering methods that consider
     # _Route53Records equivalent if they have the same class, fqdn, and _type.
     # Values are ignored. This is useful when computing diffs/changes.
 
@@ -163,17 +156,34 @@ class _Route53Record(object):
         'sub-classes should never use this method'
         return '{}:{}'.format(self.fqdn, self._type).__hash__()
 
-    def __cmp__(self, other):
-        '''sub-classes should call up to this and return its value if non-zero.
-        When it's zero they should compute their own __cmp__'''
-        if self.__class__ != other.__class__:
-            return cmp(self.__class__, other.__class__)
-        elif self.fqdn != other.fqdn:
-            return cmp(self.fqdn, other.fqdn)
-        elif self._type != other._type:
-            return cmp(self._type, other._type)
-        # We're ignoring ttl, it's not an actual differentiator
-        return 0
+    def __eq__(self, other):
+        '''Sub-classes should call up to this and return its value if true.
+        When it's false they should compute their own __eq__, same for other
+        ordering methods.'''
+        return self.__class__.__name__ == other.__class__.__name__ and \
+            self.fqdn == other.fqdn and \
+            self._type == other._type
+
+    def __ne__(self, other):
+        return self.__class__.__name__ != other.__class__.__name__ or \
+            self.fqdn != other.fqdn or \
+            self._type != other._type
+
+    def __lt__(self, other):
+        return (((self.__class__.__name__, self.fqdn, self._type)) <
+                ((other.__class__.__name__, other.fqdn, other._type)))
+
+    def __le__(self, other):
+        return (((self.__class__.__name__, self.fqdn, self._type)) <=
+                ((other.__class__.__name__, other.fqdn, other._type)))
+
+    def __gt__(self, other):
+        return (((self.__class__.__name__, self.fqdn, self._type)) >
+                ((other.__class__.__name__, other.fqdn, other._type)))
+
+    def __ge__(self, other):
+        return (((self.__class__.__name__, self.fqdn, self._type)) >=
+                ((other.__class__.__name__, other.fqdn, other._type)))
 
     def __repr__(self):
         return '_Route53Record<{} {} {} {}>'.format(self.fqdn, self._type,
@@ -514,11 +524,50 @@ class _Route53GeoRecord(_Route53Record):
         return '{}:{}:{}'.format(self.fqdn, self._type,
                                  self.geo.code).__hash__()
 
-    def __cmp__(self, other):
-        ret = super(_Route53GeoRecord, self).__cmp__(other)
-        if ret != 0:
-            return ret
-        return cmp(self.geo.code, other.geo.code)
+    def __eq__(self, other):
+        return super(_Route53GeoRecord, self).__eq__(other) and \
+            self.geo.code == other.geo.code
+
+    def __ne__(self, other):
+        # super will handle class != class, so if it's true we have 2 geo
+        # objects with the same name and type, so just need to compare codes
+        return super(_Route53GeoRecord, self).__ne__(other) or \
+            self.geo.code != other.geo.code
+
+    def __lt__(self, other):
+        # super eq will check class, name, and type
+        if super(_Route53GeoRecord, self).__eq__(other):
+            # if it's True we're dealing with two geo's with the same name and
+            # type, so we just need to compare codes
+            return self.geo.code < other.geo.code
+        # Super is not equal so we'll just let it decide lt
+        return super(_Route53GeoRecord, self).__lt__(other)
+
+    def __le__(self, other):
+        # super eq will check class, name, and type
+        if super(_Route53GeoRecord, self).__eq__(other):
+            # Just need to compare codes, everything else is equal
+            return self.geo.code <= other.geo.code
+        # Super is not equal so geo.code doesn't matter, let it decide with lt,
+        # can't be eq
+        return super(_Route53GeoRecord, self).__lt__(other)
+
+    def __gt__(self, other):
+        # super eq will check class, name, and type
+        if super(_Route53GeoRecord, self).__eq__(other):
+            # Just need to compare codes, everything else is equal
+            return self.geo.code > other.geo.code
+        # Super is not equal so we'll just let it decide gt
+        return super(_Route53GeoRecord, self).__gt__(other)
+
+    def __ge__(self, other):
+        # super eq will check class, name, and type
+        if super(_Route53GeoRecord, self).__eq__(other):
+            # Just need to compare codes, everything else is equal
+            return self.geo.code >= other.geo.code
+        # Super is not equal so geo.code doesn't matter, let it decide with gt,
+        # can't be eq
+        return super(_Route53GeoRecord, self).__gt__(other)
 
     def __repr__(self):
         return '_Route53GeoRecord<{} {} {} {} {}>'.format(self.fqdn,
@@ -561,7 +610,10 @@ def _mod_keyer(mod):
     if rrset.get('GeoLocation', False):
         unique_id = rrset['SetIdentifier']
     else:
-        unique_id = rrset['Name']
+        try:
+            unique_id = '{}-{}'.format(rrset['Name'], rrset['SetIdentifier'])
+        except KeyError:
+            unique_id = rrset['Name']
 
     # Prioritise within the action_priority, ensuring targets come first.
     if rrset.get('GeoLocation', False):
@@ -708,7 +760,7 @@ class Route53Provider(BaseProvider):
             if cc == '*':
                 # This is the default
                 return
-            cn = cca_to_ctca2(cc)
+            cn = country_alpha2_to_continent_code(cc)
             try:
                 return '{}-{}-{}'.format(cn, cc, loc['SubdivisionCode'])
             except KeyError:

requirements.txt

@@ -10,7 +10,6 @@ futures==3.2.0; python_version < '3.0'
 edgegrid-python==1.1.1
 google-cloud-core==0.28.1
 google-cloud-dns==0.29.0
-incf.countryutils==1.0
 ipaddress==1.0.22
 jmespath==0.9.3
 msrestazure==0.6.2

tests/test_octodns_provider_rackspace.py

@@ -40,7 +40,6 @@ with open('./tests/fixtures/rackspace-sample-recordset-page2.json') as fh:
 
 class TestRackspaceProvider(TestCase):
     def setUp(self):
-        self.maxDiff = 1000
         with requests_mock() as mock:
             mock.post(ANY, status_code=200, text=AUTH_RESPONSE)
             self.provider = RackspaceProvider('identity', 'test', 'api-key',

tests/test_octodns_provider_route53.py

@@ -2091,6 +2091,58 @@ class TestRoute53Records(TestCase):
         e.__repr__()
         f.__repr__()
 
+    def test_route53_record_ordering(self):
+        # Matches
+        a = _Route53Record(None, self.record_a, False)
+        b = _Route53Record(None, self.record_a, False)
+        self.assertTrue(a == b)
+        self.assertFalse(a != b)
+        self.assertFalse(a < b)
+        self.assertTrue(a <= b)
+        self.assertFalse(a > b)
+        self.assertTrue(a >= b)
+
+        # Change the fqdn is greater
+        fqdn = _Route53Record(None, self.record_a, False,
+                              fqdn_override='other')
+        self.assertFalse(a == fqdn)
+        self.assertTrue(a != fqdn)
+        self.assertFalse(a < fqdn)
+        self.assertFalse(a <= fqdn)
+        self.assertTrue(a > fqdn)
+        self.assertTrue(a >= fqdn)
+
+        provider = DummyProvider()
+        geo_a = _Route53GeoRecord(provider, self.record_a, 'NA-US',
+                                  self.record_a.geo['NA-US'], False)
+        geo_b = _Route53GeoRecord(provider, self.record_a, 'NA-US',
+                                  self.record_a.geo['NA-US'], False)
+        self.assertTrue(geo_a == geo_b)
+        self.assertFalse(geo_a != geo_b)
+        self.assertFalse(geo_a < geo_b)
+        self.assertTrue(geo_a <= geo_b)
+        self.assertFalse(geo_a > geo_b)
+        self.assertTrue(geo_a >= geo_b)
+
+        # Other base
+        geo_fqdn = _Route53GeoRecord(provider, self.record_a, 'NA-US',
+                                     self.record_a.geo['NA-US'], False)
+        geo_fqdn.fqdn = 'other'
+        self.assertFalse(geo_a == geo_fqdn)
+        self.assertTrue(geo_a != geo_fqdn)
+        self.assertFalse(geo_a < geo_fqdn)
+        self.assertFalse(geo_a <= geo_fqdn)
+        self.assertTrue(geo_a > geo_fqdn)
+        self.assertTrue(geo_a >= geo_fqdn)
+
+        # Other class
+        self.assertFalse(a == geo_a)
+        self.assertTrue(a != geo_a)
+        self.assertFalse(a < geo_a)
+        self.assertFalse(a <= geo_a)
+        self.assertTrue(a > geo_a)
+        self.assertTrue(a >= geo_a)
+
     def test_dynamic_value_delete(self):
         provider = DummyProvider()
         geo = _Route53DynamicValue(provider, self.record_a, 'iad', '2.2.2.2',
@@ -2207,70 +2259,112 @@ class TestRoute53Records(TestCase):
                                              creating=True)
         self.assertEquals(18, len(route53_records))
 
+        expected_mods = [r.mod('CREATE', []) for r in route53_records]
+        # Sort so that we get a consistent order and don't rely on set ordering
+        expected_mods.sort(key=_mod_keyer)
+
         # Convert the route53_records into mods
         self.assertEquals([{
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'HealthCheckId': 'hc42',
                 'Name': '_octodns-ap-southeast-1-value.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.4.1.2'}],
-                'SetIdentifier': 'ap-southeast-1-001',
+                'ResourceRecords': [{'Value': '1.4.1.1'}],
+                'SetIdentifier': 'ap-southeast-1-000',
                 'TTL': 60,
                 'Type': 'A',
-                'Weight': 2
-            }
+                'Weight': 2}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'HealthCheckId': 'hc42',
                 'Name': '_octodns-ap-southeast-1-value.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.4.1.1'}],
-                'SetIdentifier': 'ap-southeast-1-000',
+                'ResourceRecords': [{'Value': '1.4.1.2'}],
+                'SetIdentifier': 'ap-southeast-1-001',
                 'TTL': 60,
                 'Type': 'A',
-                'Weight': 2
-            }
+                'Weight': 2}
+        }, {
+            'Action': 'CREATE',
+            'ResourceRecordSet': {
+                'Name': '_octodns-default-pool.unit.tests.',
+                'ResourceRecords': [
+                    {'Value': '1.1.2.1'},
+                    {'Value': '1.1.2.2'}],
+                'TTL': 60,
+                'Type': 'A'}
+        }, {
+            'Action': 'CREATE',
+            'ResourceRecordSet': {
+                'HealthCheckId': 'hc42',
+                'Name': '_octodns-eu-central-1-value.unit.tests.',
+                'ResourceRecords': [{'Value': '1.3.1.1'}],
+                'SetIdentifier': 'eu-central-1-000',
+                'TTL': 60,
+                'Type': 'A',
+                'Weight': 1}
+        }, {
+            'Action': 'CREATE',
+            'ResourceRecordSet': {
+                'HealthCheckId': 'hc42',
+                'Name': '_octodns-eu-central-1-value.unit.tests.',
+                'ResourceRecords': [{'Value': '1.3.1.2'}],
+                'SetIdentifier': 'eu-central-1-001',
+                'TTL': 60,
+                'Type': 'A',
+                'Weight': 1}
+        }, {
+            'Action': 'CREATE',
+            'ResourceRecordSet': {
+                'HealthCheckId': 'hc42',
+                'Name': '_octodns-us-east-1-value.unit.tests.',
+                'ResourceRecords': [{'Value': '1.5.1.1'}],
+                'SetIdentifier': 'us-east-1-000',
+                'TTL': 60,
+                'Type': 'A',
+                'Weight': 1}
+        }, {
+            'Action': 'CREATE',
+            'ResourceRecordSet': {
+                'HealthCheckId': 'hc42',
+                'Name': '_octodns-us-east-1-value.unit.tests.',
+                'ResourceRecords': [{'Value': '1.5.1.2'}],
+                'SetIdentifier': 'us-east-1-001',
+                'TTL': 60,
+                'Type': 'A',
+                'Weight': 1}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.',
+                    'DNSName': '_octodns-ap-southeast-1-value.unit.tests.',
                     'EvaluateTargetHealth': True,
-                    'HostedZoneId': 'z45'
-                },
-                'GeoLocation': {
-                    'CountryCode': 'JP'},
-                'Name': 'unit.tests.',
-                'SetIdentifier': '0-ap-southeast-1-AS-JP',
-                'Type': 'A'
-            }
+                    'HostedZoneId': 'z45'},
+                'Failover': 'PRIMARY',
+                'Name': '_octodns-ap-southeast-1-pool.unit.tests.',
+                'SetIdentifier': 'ap-southeast-1-Primary',
+                'Type': 'A'}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-eu-central-1-pool.unit.tests.',
+                    'DNSName': '_octodns-eu-central-1-value.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'GeoLocation': {
-                    'CountryCode': 'US',
-                    'SubdivisionCode': 'FL',
-                },
-                'Name': 'unit.tests.',
-                'SetIdentifier': '1-eu-central-1-NA-US-FL',
+                'Failover': 'PRIMARY',
+                'Name': '_octodns-eu-central-1-pool.unit.tests.',
+                'SetIdentifier': 'eu-central-1-Primary',
                 'Type': 'A'}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-us-east-1-pool.unit.tests.',
+                    'DNSName': '_octodns-us-east-1-value.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'GeoLocation': {
-                    'CountryCode': '*'},
-                'Name': 'unit.tests.',
-                'SetIdentifier': '2-us-east-1-None',
+                'Failover': 'PRIMARY',
+                'Name': '_octodns-us-east-1-pool.unit.tests.',
+                'SetIdentifier': 'us-east-1-Primary',
                 'Type': 'A'}
         }, {
             'Action': 'CREATE',
@@ -2287,123 +2381,72 @@ class TestRoute53Records(TestCase):
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.',
+                    'DNSName': '_octodns-us-east-1-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'GeoLocation': {
-                    'CountryCode': 'CN'},
-                'Name': 'unit.tests.',
-                'SetIdentifier': '0-ap-southeast-1-AS-CN',
+                'Failover': 'SECONDARY',
+                'Name': '_octodns-eu-central-1-pool.unit.tests.',
+                'SetIdentifier': 'eu-central-1-Secondary-us-east-1',
                 'Type': 'A'}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-us-east-1-value.unit.tests.',
+                    'DNSName': '_octodns-default-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'Failover': 'PRIMARY',
+                'Failover': 'SECONDARY',
                 'Name': '_octodns-us-east-1-pool.unit.tests.',
-                'SetIdentifier': 'us-east-1-Primary',
+                'SetIdentifier': 'us-east-1-Secondary-default',
                 'Type': 'A'}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-eu-central-1-pool.unit.tests.',
+                    'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
                 'GeoLocation': {
-                    'ContinentCode': 'EU'},
+                    'CountryCode': 'CN'},
                 'Name': 'unit.tests.',
-                'SetIdentifier': '1-eu-central-1-EU',
+                'SetIdentifier': '0-ap-southeast-1-AS-CN',
                 'Type': 'A'}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-eu-central-1-value.unit.tests.',
+                    'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'Failover': 'PRIMARY',
-                'Name': '_octodns-eu-central-1-pool.unit.tests.',
-                'SetIdentifier': 'eu-central-1-Primary',
-                'Type': 'A'}
-        }, {
-            'Action': 'CREATE',
-            'ResourceRecordSet': {
-                'Name': '_octodns-default-pool.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.1.2.1'},
-                    {
-                        'Value': '1.1.2.2'}],
-                'TTL': 60,
+                'GeoLocation': {
+                    'CountryCode': 'JP'},
+                'Name': 'unit.tests.',
+                'SetIdentifier': '0-ap-southeast-1-AS-JP',
                 'Type': 'A'}
-        }, {
-            'Action': 'CREATE',
-            'ResourceRecordSet': {
-                'HealthCheckId': 'hc42',
-                'Name': '_octodns-eu-central-1-value.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.3.1.2'}],
-                'SetIdentifier': 'eu-central-1-001',
-                'TTL': 60,
-                'Type': 'A',
-                'Weight': 1}
-        }, {
-            'Action': 'CREATE',
-            'ResourceRecordSet': {
-                'HealthCheckId': 'hc42',
-                'Name': '_octodns-eu-central-1-value.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.3.1.1'}],
-                'SetIdentifier': 'eu-central-1-000',
-                'TTL': 60,
-                'Type': 'A',
-                'Weight': 1}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-default-pool.unit.tests.',
+                    'DNSName': '_octodns-eu-central-1-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'Failover': 'SECONDARY',
-                'Name': '_octodns-us-east-1-pool.unit.tests.',
-                'SetIdentifier': 'us-east-1-Secondary-default',
+                'GeoLocation': {
+                    'ContinentCode': 'EU'},
+                'Name': 'unit.tests.',
+                'SetIdentifier': '1-eu-central-1-EU',
                 'Type': 'A'}
-        }, {
-            'Action': 'CREATE',
-            'ResourceRecordSet': {
-                'HealthCheckId': 'hc42',
-                'Name': '_octodns-us-east-1-value.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.5.1.2'}],
-                'SetIdentifier': 'us-east-1-001',
-                'TTL': 60,
-                'Type': 'A',
-                'Weight': 1}
-        }, {
-            'Action': 'CREATE',
-            'ResourceRecordSet': {
-                'HealthCheckId': 'hc42',
-                'Name': '_octodns-us-east-1-value.unit.tests.',
-                'ResourceRecords': [{
-                    'Value': '1.5.1.1'}],
-                'SetIdentifier': 'us-east-1-000',
-                'TTL': 60,
-                'Type': 'A',
-                'Weight': 1}
         }, {
             'Action': 'CREATE',
             'ResourceRecordSet': {
                 'AliasTarget': {
-                    'DNSName': '_octodns-ap-southeast-1-value.unit.tests.',
+                    'DNSName': '_octodns-eu-central-1-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'Failover': 'PRIMARY',
-                'Name': '_octodns-ap-southeast-1-pool.unit.tests.',
-                'SetIdentifier': 'ap-southeast-1-Primary',
+                'GeoLocation': {
+                    'CountryCode': 'US',
+                    'SubdivisionCode': 'FL'},
+                'Name': 'unit.tests.',
+                'SetIdentifier': '1-eu-central-1-NA-US-FL',
                 'Type': 'A'}
         }, {
             'Action': 'CREATE',
@@ -2412,11 +2455,12 @@ class TestRoute53Records(TestCase):
                     'DNSName': '_octodns-us-east-1-pool.unit.tests.',
                     'EvaluateTargetHealth': True,
                     'HostedZoneId': 'z45'},
-                'Failover': 'SECONDARY',
-                'Name': '_octodns-eu-central-1-pool.unit.tests.',
-                'SetIdentifier': 'eu-central-1-Secondary-us-east-1',
+                'GeoLocation': {
+                    'CountryCode': '*'},
+                'Name': 'unit.tests.',
+                'SetIdentifier': '2-us-east-1-None',
                 'Type': 'A'}
-        }], [r.mod('CREATE', []) for r in route53_records])
+        }], expected_mods)
 
         for route53_record in route53_records:
             # Smoke test stringification