More thorough testing of root ns cases and associated improvements

octodns/provider/base.py

@@ -98,12 +98,13 @@ class BaseProvider(BaseSource):
         record = desired.root_ns
         if self.SUPPORTS_ROOT_NS:
             if not record:
-                self.log.warning('%s: root NS record supported by provider, '
-                                 'but no record is configured for %s', self.id,
-                                 desired.name)
+                msg = 'root NS record supported, but no record is ' \
+                    f'configured for {desired.name}'
+                fallback = 'ignoring it'
+                self.supports_warn_or_except(msg, fallback)
         else:
             if record:
-                # ignore, we can't manage root NS records
+                # we can't manage root NS records, get rid of it
                 msg = \
                     f'root NS record not supported for {record.fqdn}'
                 fallback = 'ignoring it'
@@ -133,16 +134,10 @@ class BaseProvider(BaseSource):
         '''
 
         existing_root_ns = existing.root_ns
-        if existing_root_ns and (not desired.root_ns or not
-                                 self.SUPPORTS_ROOT_NS):
-            # we have an existing root NS record and either the provider
-            # doesn't support managing them or our desired state doesn't
-            # include one, either way we'll exclude the existing one from
-            # consideration
-            msg = \
-                f'root NS record not supported for {existing_root_ns.fqdn}'
-            fallback = 'ignoring it'
-            self.supports_warn_or_except(msg, fallback)
+        if existing_root_ns and (not self.SUPPORTS_ROOT_NS or
+                                 not desired.root_ns):
+            self.log.info('root NS record in existing, but not supported or '
+                          'not configured; ignoring it')
             existing.remove_record(existing_root_ns)
 
         return existing

tests/test_octodns_provider_base.py

@@ -707,6 +707,7 @@ class TestBaseProviderSupportsRootNs(TestCase):
         self.assertFalse(plan)
 
         # plan again with strict_supports enabled, we should get an exception
+        # b/c we have something configured that can't be managed
         provider.strict_supports = True
         with self.assertRaises(SupportsException) as ctx:
             provider.plan(self.has_root)
@@ -725,6 +726,15 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # anyway, they will have been removed from the desired and existing.
         self.assertFalse(plan)
 
+        # plan again with strict_supports enabled, we should get an exception
+        # b/c we have something configured that can't be managed (doesn't
+        # matter that it's a mis-match)
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.has_root)
+        self.assertEqual('test: root NS record not supported for unit.tests.',
+                         str(ctx.exception))
+
     def test_supports_root_ns_false_missing(self):
         # provider has an existing record
         provider = self.Provider(self.has_root)
@@ -737,6 +747,12 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # anyway, they will have been removed from the desired and existing.
         self.assertFalse(plan)
 
+        # plan again with strict supports enabled, no change since desired
+        # isn't asking to manage root
+        provider.strict_supports = True
+        plan = provider.plan(self.no_root)
+        self.assertFalse(plan)
+
     def test_supports_root_ns_false_create_zone(self):
         # provider has no existing records (create)
         provider = self.Provider()
@@ -749,6 +765,33 @@ class TestBaseProviderSupportsRootNs(TestCase):
         self.assertTrue(plan)
         self.assertEqual(2, len(plan.changes))
 
+        # plan again with strict supports enabled, we'll get an exception b/c
+        # the target provider can't manage something in desired
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.has_root)
+        self.assertEqual('test: root NS record not supported for unit.tests.',
+                         str(ctx.exception))
+
+    def test_supports_root_ns_false_create_zone_missing(self):
+        # provider has no existing records (create)
+        provider = self.Provider()
+        provider.SUPPORTS_ROOT_NS = False
+
+        # case where we have a root NS in the desired
+        plan = provider.plan(self.no_root)
+
+        # no support for root NS so we only create the other two records
+        self.assertTrue(plan)
+        self.assertEqual(2, len(plan.changes))
+
+        # plan again with strict supports enabled, same result since we're not
+        # asking for a root NS it's just the 2 other changes
+        provider.strict_supports = True
+        plan = provider.plan(self.no_root)
+        self.assertTrue(plan)
+        self.assertEqual(2, len(plan.changes))
+
     # True
 
     def test_supports_root_ns_true_matches(self):
@@ -763,6 +806,11 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # change
         self.assertFalse(plan)
 
+        # again with strict supports enabled, no difference
+        provider.strict_supports = True
+        plan = provider.plan(self.has_root)
+        self.assertFalse(plan)
+
     def test_supports_root_ns_true_different(self):
         # provider has a non-matching existing record
         provider = self.Provider(self.different_root)
@@ -778,6 +826,14 @@ class TestBaseProviderSupportsRootNs(TestCase):
         self.assertEqual(self.other_root_ns_record, change.existing)
         self.assertEqual(self.root_ns_record, change.new)
 
+        # again with strict supports enabled, no difference, we see the change
+        provider.strict_supports = True
+        plan = provider.plan(self.has_root)
+        self.assertTrue(plan)
+        change = plan.changes[0]
+        self.assertEqual(self.other_root_ns_record, change.existing)
+        self.assertEqual(self.root_ns_record, change.new)
+
     def test_supports_root_ns_true_missing(self):
         # provider has a matching existing root record
         provider = self.Provider(self.has_root)
@@ -790,6 +846,14 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # aren't configured with one to manage
         self.assertFalse(plan)
 
+        # again with strict supports enabled, this time we throw an exception
+        # b/c it's not being managed and could be
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.no_root)
+        self.assertEqual('test: root NS record supported, but no record is '
+                         'configured for unit.tests.', str(ctx.exception))
+
     def test_supports_root_ns_true_create_zone(self):
         # provider has no existing records (create)
         provider = self.Provider()
@@ -807,6 +871,17 @@ class TestBaseProviderSupportsRootNs(TestCase):
         self.assertFalse(change.existing)
         self.assertEqual(self.root_ns_record, change.new)
 
+        # again with strict supports enabled, no difference, we see all 3
+        # changes
+        provider.strict_supports = True
+        plan = provider.plan(self.has_root)
+        self.assertTrue(plan)
+        self.assertEqual(3, len(plan.changes))
+        change = [c for c in plan.changes
+                  if c.new.name == '' and c.new._type == 'NS'][0]
+        self.assertFalse(change.existing)
+        self.assertEqual(self.root_ns_record, change.new)
+
     def test_supports_root_ns_true_create_zone_missing(self):
         # provider has no existing records (create)
         provider = self.Provider()
@@ -816,3 +891,11 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # manage the other records
         plan = provider.plan(self.no_root)
         self.assertEqual(2, len(plan.changes))
+
+        # again with strict supports enabled, this time we throw an exception
+        # b/c it's not being managed and could be
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.no_root)
+        self.assertEqual('test: root NS record supported, but no record is '
+                         'configured for unit.tests.', str(ctx.exception))