Merge pull request #1223 from octodns/yaml-plan-and-apply

YamlProvider rework to be fully plan and apply compliant

octodns/provider/yaml.py

@@ -73,6 +73,12 @@ class YamlProvider(BaseProvider):
         # (optional, default False)
         disable_zonefile: false
 
+    Note
+    ----
+
+    When using this provider as a target any existing comments or formatting
+    in the zone files will be lost when changes are applyed.
+
     Split Details
     -------------
 
@@ -342,11 +348,6 @@ class YamlProvider(BaseProvider):
             lenient,
         )
 
-        if target:
-            # When acting as a target we ignore any existing records so that we
-            # create a completely new copy
-            return False
-
         before = len(zone.records)
 
         sources = []
@@ -363,33 +364,37 @@ class YamlProvider(BaseProvider):
         if self.shared_filename:
             sources.append(join(self.directory, self.shared_filename))
 
-        if not sources:
+        if not sources and not target:
             raise ProviderException(f'no YAMLs found for {zone.decoded_name}')
 
-        # determinstically order our sources
+        # deterministically order our sources
         sources.sort()
 
         for source in sources:
             self._populate_from_file(source, zone, lenient)
 
+        exists = len(sources) > 0
         self.log.info(
-            'populate:   found %s records, exists=False',
+            'populate:   found %s records, exists=%s',
             len(zone.records) - before,
+            exists,
         )
-        return False
+        return exists
 
     def _apply(self, plan):
-        desired = plan.desired
+        # make a copy of existing we can muck with
+        copy = plan.existing.copy()
         changes = plan.changes
         self.log.debug(
-            '_apply: zone=%s, len(changes)=%d',
-            desired.decoded_name,
-            len(changes),
+            '_apply: zone=%s, len(changes)=%d', copy.decoded_name, len(changes)
         )
-        # Since we don't have existing we'll only see creates
-        records = [c.new for c in changes]
-        # Order things alphabetically (records sort that way
-        records.sort()
+
+        # apply our pending changes to that copy
+        copy.apply(changes)
+
+        # we now have the records we need to write out, order things
+        # alphabetically (records sort that way
+        records = sorted(copy.records)
         data = defaultdict(list)
         for record in records:
             d = record.data
@@ -411,7 +416,7 @@ class YamlProvider(BaseProvider):
 
         if self.split_extension:
             # we're going to do split files
-            decoded_name = desired.decoded_name[:-1]
+            decoded_name = copy.decoded_name[:-1]
             directory = join(
                 self.directory, f'{decoded_name}{self.split_extension}'
             )
@@ -443,7 +448,7 @@ class YamlProvider(BaseProvider):
 
         else:
             # single large file
-            filename = join(self.directory, f'{desired.decoded_name}yaml')
+            filename = join(self.directory, f'{copy.decoded_name}yaml')
             self.log.debug('_apply:   writing filename=%s', filename)
             with open(filename, 'w') as fh:
                 safe_dump(

octodns/zone.py

@@ -340,6 +340,16 @@ class Zone(object):
 
         return changes
 
+    def apply(self, changes):
+        '''
+        Apply the provided changes to the zone.
+        '''
+        for change in changes:
+            if isinstance(change, Delete):
+                self.remove_record(change.existing)
+            else:
+                self.add_record(change.new, replace=True, lenient=True)
+
     def hydrate(self):
         '''
         Take a shallow copy Zone and make it a deeper copy holding its own

tests/test_octodns_manager.py

@@ -2,7 +2,7 @@
 #
 #
 
-from os import environ, listdir
+from os import environ, listdir, remove
 from os.path import dirname, isfile, join
 from unittest import TestCase
 from unittest.mock import MagicMock, patch
@@ -39,6 +39,13 @@ def get_config_filename(which):
     return join(config_dir, which)
 
 
+def reset(directory):
+    for filename in listdir(directory):
+        if filename.endswith('.yaml'):
+            filename = join(directory, filename)
+            remove(filename)
+
+
 class TestManager(TestCase):
     def test_missing_provider_class(self):
         with self.assertRaises(ManagerException) as ctx:
@@ -147,40 +154,47 @@ class TestManager(TestCase):
         with TemporaryDirectory() as tmpdir:
             environ['YAML_TMP_DIR'] = tmpdir.dirname
             environ['YAML_TMP_DIR2'] = tmpdir.dirname
+
             tc = Manager(get_config_filename('simple.yaml')).sync(dry_run=False)
             self.assertEqual(28, tc)
 
             # try with just one of the zones
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, eligible_zones=['unit.tests.']
             )
             self.assertEqual(22, tc)
 
             # the subzone, with 2 targets
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, eligible_zones=['subzone.unit.tests.']
             )
             self.assertEqual(6, tc)
 
             # and finally the empty zone
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, eligible_zones=['empty.']
             )
             self.assertEqual(0, tc)
 
             # Again with force
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, force=True
             )
             self.assertEqual(28, tc)
 
             # Again with max_workers = 1
+            reset(tmpdir.dirname)
             tc = Manager(
                 get_config_filename('simple.yaml'), max_workers=1
             ).sync(dry_run=False, force=True)
             self.assertEqual(28, tc)
 
             # Include meta
+            reset(tmpdir.dirname)
             tc = Manager(
                 get_config_filename('simple.yaml'),
                 max_workers=1,
@@ -1011,22 +1025,23 @@ class TestManager(TestCase):
         )
 
     def test_auto_arpa(self):
-        manager = Manager(get_config_filename('simple-arpa.yaml'))
+        with TemporaryDirectory() as tmpdir:
+            environ['YAML_TMP_DIR'] = tmpdir.dirname
 
-        # provider config
-        self.assertEqual(
-            True, manager.providers.get("auto-arpa").populate_should_replace
-        )
-        self.assertEqual(1800, manager.providers.get("auto-arpa").ttl)
+            manager = Manager(get_config_filename('simple-arpa.yaml'))
 
-        # processor config
-        self.assertEqual(
-            True, manager.processors.get("auto-arpa").populate_should_replace
-        )
-        self.assertEqual(1800, manager.processors.get("auto-arpa").ttl)
+            # provider config
+            self.assertEqual(
+                True, manager.providers.get("auto-arpa").populate_should_replace
+            )
+            self.assertEqual(1800, manager.providers.get("auto-arpa").ttl)
 
-        with TemporaryDirectory() as tmpdir:
-            environ['YAML_TMP_DIR'] = tmpdir.dirname
+            # processor config
+            self.assertEqual(
+                True,
+                manager.processors.get("auto-arpa").populate_should_replace,
+            )
+            self.assertEqual(1800, manager.processors.get("auto-arpa").ttl)
 
             # we can sync eligible_zones so long as they're not arpa
             tc = manager.sync(dry_run=False, eligible_zones=['unit.tests.'])
@@ -1043,6 +1058,7 @@ class TestManager(TestCase):
             )
 
             # same for eligible_sources
+            reset(tmpdir.dirname)
             tc = manager.sync(
                 dry_run=False,
                 eligible_zones=['unit.tests.'],
@@ -1058,6 +1074,7 @@ class TestManager(TestCase):
             )
 
             # same for eligible_targets
+            reset(tmpdir.dirname)
             tc = manager.sync(
                 dry_run=False,
                 eligible_zones=['unit.tests.'],
@@ -1073,10 +1090,11 @@ class TestManager(TestCase):
             )
 
             # full sync with arpa is fine, 2 extra records from it
+            reset(tmpdir.dirname)
             tc = manager.sync(dry_run=False)
             self.assertEqual(26, tc)
 
-    def test_dynamic_config(self):
+    def test_dynamic_config_targeted(self):
         with TemporaryDirectory() as tmpdir:
             environ['YAML_TMP_DIR'] = tmpdir.dirname
 
@@ -1100,6 +1118,12 @@ class TestManager(TestCase):
                 ),
             )
 
+    def test_dynamic_config_all(self):
+        with TemporaryDirectory() as tmpdir:
+            environ['YAML_TMP_DIR'] = tmpdir.dirname
+
+            manager = Manager(get_config_filename('dynamic-config.yaml'))
+
             # should sync everything across all zones, total of 32 records
             self.assertEqual(32, manager.sync(dry_run=False))
 

tests/test_octodns_provider_yaml.py

@@ -29,11 +29,12 @@ class TestYamlProvider(TestCase):
         zone = Zone('unit.tests.', [])
         dynamic_zone = Zone('dynamic.tests.', [])
 
-        # With target we don't add anything
+        # With target we see everything
         source.populate(zone, target=source)
-        self.assertEqual(0, len(zone.records))
+        self.assertEqual(25, len(zone.records))
 
         # without it we see everything
+        zone = Zone('unit.tests.', [])
         source.populate(zone)
         self.assertEqual(25, len(zone.records))
 
@@ -95,11 +96,9 @@ class TestYamlProvider(TestCase):
 
             self.assertFalse(zone.changes(reloaded, target=source))
 
-            # A 2nd sync should still create everything
+            # A 2nd sync should result in no changes, thus no plan
             plan = target.plan(zone)
-            self.assertEqual(
-                22, len([c for c in plan.changes if isinstance(c, Create)])
-            )
+            self.assertFalse(plan)
 
             with open(yaml_file) as fh:
                 data = safe_load(fh.read())
@@ -177,7 +176,8 @@ class TestYamlProvider(TestCase):
             zone = Zone(idna_encode(name), [])
 
             # create a idna named file
-            with open(join(td.dirname, idna_encode(filename)), 'w') as fh:
+            idna_filename = join(td.dirname, idna_encode(filename))
+            with open(idna_filename, 'w') as fh:
                 fh.write(
                     '''---
 '':
@@ -204,7 +204,14 @@ xn--dj-kia8a:
             self.assertEqual(['2.3.4.5'], d['xn--dj-kia8a'].values)
             self.assertEqual(['3.4.5.6'], d['xn--28jm5b5a8k5k8cra'].values)
 
-            # create a utf8 named file (provider always writes utf-8 filenames
+            # if we plan there'll be nothing to do
+            plan = provider.plan(zone)
+            self.assertFalse(plan)
+
+            # get rid of the idna file
+            remove(idna_filename)
+            # create a utf8 named file (provider always writes utf-8 filenames,
+            # no file should have a plan now
             plan = provider.plan(zone)
             provider.apply(plan)
 
@@ -214,6 +221,9 @@ xn--dj-kia8a:
                 self.assertTrue('déjà:' in content)
                 self.assertTrue('これはテストです:' in content)
 
+            # recreate the idna version of the file
+            with open(idna_filename, 'w') as fh:
+                fh.write('')
             # does not allow both idna and utf8 named files
             with self.assertRaises(ProviderException) as ctx:
                 provider.populate(zone)
@@ -495,11 +505,12 @@ class TestSplitYamlProvider(TestCase):
         zone = Zone('unit.tests.', [])
         dynamic_zone = Zone('dynamic.tests.', [])
 
-        # With target we don't add anything
+        # With target we still see whatever is in the file
         source.populate(zone, target=source)
-        self.assertEqual(0, len(zone.records))
+        self.assertEqual(20, len(zone.records))
 
-        # without it we see everything
+        # without it we see everything too, doesn't make a difference
+        zone = Zone('unit.tests.', [])
         source.populate(zone)
         self.assertEqual(20, len(zone.records))
         self.assertFalse([r for r in zone.records if r.name.startswith('only')])
@@ -586,11 +597,9 @@ class TestSplitYamlProvider(TestCase):
 
             self.assertFalse(zone.changes(reloaded, target=source))
 
-            # A 2nd sync should still create everything
+            # A 2nd sync should have nothing to do
             plan = target.plan(zone)
-            self.assertEqual(
-                17, len([c for c in plan.changes if isinstance(c, Create)])
-            )
+            self.assertFalse(plan)
 
             yaml_file = join(zone_dir, '$unit.tests.yaml')
             self.assertTrue(isfile(yaml_file))

tests/test_octodns_zone.py

@@ -179,6 +179,9 @@ class TestZone(TestCase):
 
         # before == after -> no changes
         self.assertFalse(before.changes(after, target))
+        copy = before.copy()
+        copy.apply([])
+        self.assertEqual(copy.records, before.records)
 
         # add a record, delete a record -> [Delete, Create]
         c = ARecord(before, 'c', {'ttl': 42, 'value': '1.1.1.1'})
@@ -199,6 +202,15 @@ class TestZone(TestCase):
         delete.__repr__()
         create.__repr__()
 
+        # make a copy of before
+        copy = before.copy()
+        # apply the changes to it
+        copy.apply(changes)
+        # copy should not match it's origin any longer
+        self.assertNotEqual(copy.records, before.records)
+        # and it should now match the target
+        self.assertEqual(copy.records, after.records)
+
         after = Zone('unit.tests.', [])
         changed = ARecord(before, 'a', {'ttl': 42, 'value': '2.2.2.2'})
         after.add_record(changed)