Greatly simplify MetaProcessor and (more) thoroughly test it

octodns/processor/meta.py

@@ -62,11 +62,11 @@ class MetaProcessor(BaseProcessor):
     '''
 
     @classmethod
-    def now(cls):
+    def get_time(cls):
         return datetime.now(UTC).isoformat()
 
     @classmethod
-    def uuid(cls):
+    def get_uuid(cls):
         return str(uuid4())
 
     def __init__(
@@ -91,47 +91,37 @@ class MetaProcessor(BaseProcessor):
             ttl,
         )
         self.record_name = record_name
-        values = []
-        if include_time:
-            time = self.now()
-            values.append(f'time={time}')
-        if include_uuid:
-            uuid = self.uuid() if include_uuid else None
-            values.append(f'uuid={uuid}')
-        if include_version:
-            values.append(f'octodns-version={__version__}')
+        self.time = self.get_time() if include_time else None
+        self.uuid = self.get_uuid() if include_uuid else None
+        self.include_version = include_version
         self.include_provider = include_provider
-        values.sort()
-        self.values = values
         self.ttl = ttl
 
-    def process_source_zone(self, desired, sources):
+    def values(self, target_id):
+        ret = []
+        if self.include_version:
+            ret.append(f'octodns-version={__version__}')
+        if self.include_provider:
+            ret.append(f'provider={target_id}')
+        if self.time:
+            ret.append(f'time={self.time}')
+        if self.uuid:
+            ret.append(f'uuid={self.uuid}')
+        return ret
+
+    def process_source_and_target_zones(self, desired, existing, target):
         meta = Record.new(
             desired,
             self.record_name,
-            {'ttl': self.ttl, 'type': 'TXT', 'values': self.values},
+            {'ttl': self.ttl, 'type': 'TXT', 'values': self.values(target.id)},
             # we may be passing in empty values here to be filled out later in
             # process_source_and_target_zones
             lenient=True,
         )
         desired.add_record(meta)
-        return desired
-
-    def process_source_and_target_zones(self, desired, existing, target):
-        if self.include_provider:
-            # look for the meta record
-            for record in sorted(desired.records):
-                if record.name == self.record_name and record._type == 'TXT':
-                    # we've found it, make a copy we can modify
-                    record = record.copy()
-                    record.values = record.values + [f'provider={target.id}']
-                    record.values.sort()
-                    desired.add_record(record, replace=True)
-                    break
-
         return desired, existing
 
-    def _is_up_to_date_meta(self, change):
+    def _is_up_to_date_meta(self, change, target_id):
         # always something so we can see if its type and name
         record = change.record
         # existing state, if there is one
@@ -140,17 +130,19 @@ class MetaProcessor(BaseProcessor):
             record._type == 'TXT'
             and record.name == self.record_name
             and existing is not None
-            and _keys(existing.values) == _keys(self.values)
+            # don't care about the values here, just the fields/keys
+            and _keys(self.values(target_id)) == _keys(existing.values)
         )
 
     def process_plan(self, plan, sources, target):
         if (
             plan
             and len(plan.changes) == 1
-            and self._is_up_to_date_meta(plan.changes[0])
+            and self._is_up_to_date_meta(plan.changes[0], target.id)
         ):
             # the only change is the meta record, and it's not meaningfully
-            # changing so we don't actually want to make the change
+            # changing so we don't actually want to make the update, meta should
+            # only be enough to cause a plan on its own if the fields changed
             return None
 
         # There's more than one thing changing so meta should update and/or meta

tests/test_octodns_processor_meta.py

@@ -12,6 +12,13 @@ from octodns.record import Create, Record, Update
 from octodns.zone import Zone
 
 
+class DummyTarget:
+    id = 'dummy'
+
+
+dummy_target = DummyTarget()
+
+
 class TestMetaProcessor(TestCase):
     zone = Zone('unit.tests.', [])
 
@@ -54,35 +61,37 @@ class TestMetaProcessor(TestCase):
         {'type': 'CNAME', 'ttl': 61, 'value': 'points.to.something.'},
     )
 
-    @patch('octodns.processor.meta.MetaProcessor.now')
-    @patch('octodns.processor.meta.MetaProcessor.uuid')
-    def test_args_and_values(self, uuid_mock, now_mock):
+    @patch('octodns.processor.meta.MetaProcessor.get_time')
+    @patch('octodns.processor.meta.MetaProcessor.get_uuid')
+    def test_args_and_values(self, get_uuid_mock, get_time_mock):
         # defaults, just time
-        uuid_mock.side_effect = [Exception('not used')]
-        now_mock.side_effect = ['the-time']
+        get_uuid_mock.side_effect = [Exception('not used')]
+        get_time_mock.side_effect = ['the-time']
         proc = MetaProcessor('test')
-        self.assertEqual(['time=the-time'], proc.values)
+        self.assertEqual(['time=the-time'], proc.values('dummy'))
 
         # just uuid
-        uuid_mock.side_effect = ['abcdef-1234567890']
-        now_mock.side_effect = [Exception('not used')]
+        get_uuid_mock.side_effect = ['abcdef-1234567890']
+        get_time_mock.side_effect = [Exception('not used')]
         proc = MetaProcessor('test', include_time=False, include_uuid=True)
-        self.assertEqual(['uuid=abcdef-1234567890'], proc.values)
+        self.assertEqual(['uuid=abcdef-1234567890'], proc.values('dummy'))
 
         # just version
-        uuid_mock.side_effect = [Exception('not used')]
-        now_mock.side_effect = [Exception('not used')]
+        get_uuid_mock.side_effect = [Exception('not used')]
+        get_time_mock.side_effect = [Exception('not used')]
         proc = MetaProcessor('test', include_time=False, include_version=True)
-        self.assertEqual([f'octodns-version={__version__}'], proc.values)
+        self.assertEqual(
+            [f'octodns-version={__version__}'], proc.values('dummy')
+        )
 
         # just provider
         proc = MetaProcessor('test', include_time=False, include_provider=True)
         self.assertTrue(proc.include_provider)
-        self.assertFalse(proc.values)
+        self.assertEqual(['provider=dummy'], proc.values('dummy'))
 
         # everything
-        uuid_mock.side_effect = ['abcdef-1234567890']
-        now_mock.side_effect = ['the-time']
+        get_uuid_mock.side_effect = ['abcdef-1234567890']
+        get_time_mock.side_effect = ['the-time']
         proc = MetaProcessor(
             'test',
             include_time=True,
@@ -93,106 +102,110 @@ class TestMetaProcessor(TestCase):
         self.assertEqual(
             [
                 f'octodns-version={__version__}',
+                'provider=dummy-x',
                 'time=the-time',
                 'uuid=abcdef-1234567890',
             ],
-            proc.values,
+            list(proc.values('dummy-x')),
         )
         self.assertTrue(proc.include_provider)
 
     def test_uuid(self):
         proc = MetaProcessor('test', include_time=False, include_uuid=True)
-        self.assertEqual(1, len(proc.values))
-        self.assertTrue(proc.values[0].startswith('uuid'))
-        # uuid's have 4 -
-        self.assertEqual(4, proc.values[0].count('-'))
+        self.assertTrue(proc.uuid)
+        self.assertFalse(proc.time)
+        self.assertFalse(proc.include_provider)
+        self.assertFalse(proc.include_version)
+
+        values = list(proc.values('dummy'))
+        self.assertEqual(1, len(values))
+        value = values[0]
+        self.assertEqual(f'uuid={proc.uuid}', value)
 
     def test_is_up_to_date_meta(self):
         proc = MetaProcessor('test')
 
         # Creates always need to happen
         self.assertFalse(
-            proc._is_up_to_date_meta(Create(self.meta_needs_update))
+            proc._is_up_to_date_meta(Create(self.meta_needs_update), 'dummy')
+        )
+        self.assertFalse(
+            proc._is_up_to_date_meta(Create(self.meta_up_to_date), 'dummy')
         )
-        self.assertFalse(proc._is_up_to_date_meta(Create(self.meta_up_to_date)))
 
         # Updates depend on the contents
         self.assertFalse(
-            proc._is_up_to_date_meta(Update(self.meta_needs_update, None))
+            proc._is_up_to_date_meta(
+                Update(self.meta_needs_update, None), 'dummy'
+            )
         )
         self.assertTrue(
-            proc._is_up_to_date_meta(Update(self.meta_up_to_date, None))
+            proc._is_up_to_date_meta(
+                Update(self.meta_up_to_date, None), 'dummy'
+            )
         )
 
         # not a meta txt
-        self.assertFalse(proc._is_up_to_date_meta(Update(self.not_meta, None)))
+        self.assertFalse(
+            proc._is_up_to_date_meta(Update(self.not_meta, None), 'dummy')
+        )
 
         # not even a txt record
-        self.assertFalse(proc._is_up_to_date_meta(Update(self.not_txt, None)))
+        self.assertFalse(
+            proc._is_up_to_date_meta(Update(self.not_txt, None), 'dummy')
+        )
+
+    @patch('octodns.processor.meta.MetaProcessor.get_time')
+    def test_process_source_and_target_zones(self, get_time_mock):
+        get_time_mock.side_effect = [
+            'the-time',
+            'the-time-2',
+            'the-time-3',
+            'the-time-4',
+        ]
 
-    @patch('octodns.processor.meta.MetaProcessor.now')
-    def test_process_source_zone(self, now_mock):
-        now_mock.side_effect = ['the-time']
         proc = MetaProcessor('test')
+        self.assertFalse(proc.uuid)
+        self.assertTrue(proc.time)
+        self.assertFalse(proc.include_provider)
+        self.assertFalse(proc.include_version)
 
-        # meta record was added
+        existing = self.zone.copy()
         desired = self.zone.copy()
-        processed = proc.process_source_zone(desired, None)
+        processed, _ = proc.process_source_and_target_zones(
+            desired, existing, dummy_target
+        )
         record = next(iter(processed.records))
         self.assertEqual(self.meta_up_to_date, record)
         self.assertEqual(['time=the-time'], record.values)
 
-    def test_process_source_and_target_zones(self):
-        proc = MetaProcessor('test')
-
         # with defaults, not enabled
         existing = self.zone.copy()
         desired = self.zone.copy()
         processed, _ = proc.process_source_and_target_zones(
-            existing, desired, None
+            desired, existing, dummy_target
         )
-        self.assertFalse(processed.records)
+        records = processed.records
+        self.assertEqual(1, len(records))
+        record = next(iter(records))
+        self.assertEqual(proc.record_name, record.name)
+        self.assertEqual('TXT', record._type)
+        self.assertEqual(['time=the-time'], record.values)
 
         # enable provider
         proc = MetaProcessor('test', include_provider=True)
+        self.assertFalse(proc.uuid)
+        self.assertTrue(proc.time)
+        self.assertTrue(proc.include_provider)
+        self.assertFalse(proc.include_version)
 
-        class DummyTarget:
-            id = 'dummy'
-
-        # enabled provider, no meta record, shouldn't happen, but also shouldn't
-        # blow up
-        processed, _ = proc.process_source_and_target_zones(
-            existing, desired, DummyTarget()
-        )
-        self.assertFalse(processed.records)
-
-        # enabled provider, should now look for and update the provider value,
-        # - only record so nothing to skip over
-        # - time value in there to be skipped over
-        proc = MetaProcessor('test', include_provider=True)
         existing = self.zone.copy()
         desired = self.zone.copy()
-        meta = self.meta_up_to_date.copy()
-        existing.add_record(meta)
         processed, _ = proc.process_source_and_target_zones(
-            existing, desired, DummyTarget()
+            existing, desired, dummy_target
         )
         record = next(iter(processed.records))
-        self.assertEqual(['provider=dummy', 'time=xxx'], record.values)
-
-        # add another unrelated record that needs to be skipped
-        proc = MetaProcessor('test', include_provider=True)
-        existing = self.zone.copy()
-        desired = self.zone.copy()
-        meta = self.meta_up_to_date.copy()
-        existing.add_record(meta)
-        existing.add_record(self.not_meta)
-        processed, _ = proc.process_source_and_target_zones(
-            existing, desired, DummyTarget()
-        )
-        self.assertEqual(2, len(processed.records))
-        record = [r for r in processed.records if r.name == proc.record_name][0]
-        self.assertEqual(['provider=dummy', 'time=xxx'], record.values)
+        self.assertEqual(['provider=dummy', 'time=the-time-2'], record.values)
 
     def test_process_plan(self):
         proc = MetaProcessor('test')
@@ -200,23 +213,24 @@ class TestMetaProcessor(TestCase):
         # no plan, shouldn't happen, but we shouldn't blow up
         self.assertFalse(proc.process_plan(None, None, None))
 
-        # plan with just an up to date meta record, should kill off the plan
+        # plan with only a meta record that has the correct config/keys
         plan = Plan(
-            None,
-            None,
-            [Update(self.meta_up_to_date, self.meta_needs_update)],
+            None,  # ignored
+            None,  # ignored
+            [Update(self.meta_up_to_date, self.meta_up_to_date)],
             True,
         )
-        self.assertFalse(proc.process_plan(plan, None, None))
+        self.assertFalse(proc.process_plan(plan, [], dummy_target))
 
-        # plan with an out of date meta record, should leave the plan alone
+        # plan with only a meta record that has the wrong config/keys and thus
+        # needs updating
         plan = Plan(
             None,
             None,
             [Update(self.meta_needs_update, self.meta_up_to_date)],
             True,
         )
-        self.assertEqual(plan, proc.process_plan(plan, None, None))
+        self.assertEqual(plan, proc.process_plan(plan, [], dummy_target))
 
         # plan with other changes preserved even if meta was somehow up to date
         plan = Plan(
@@ -228,4 +242,43 @@ class TestMetaProcessor(TestCase):
             ],
             True,
         )
-        self.assertEqual(plan, proc.process_plan(plan, None, None))
+        self.assertEqual(plan, proc.process_plan(plan, [], dummy_target))
+
+    def test_flow(self):
+        proc = MetaProcessor(
+            'test',
+            record_name='special',
+            include_version=True,
+            include_provider=True,
+            include_time=False,
+        )
+
+        desired = self.zone.copy()
+        # start out with no records
+        self.assertFalse(desired.records)
+
+        # now process source and target zones (existing isn't touched)
+        desired, _ = proc.process_source_and_target_zones(
+            desired, [], dummy_target
+        )
+        records = desired.records
+        self.assertEqual(1, len(records))
+        meta = next(iter(records))
+        # has the expected type and name & type
+        self.assertEqual(proc.record_name, meta.name)
+        self.assertEqual('TXT', meta._type)
+        # at this point values will just have version, no provider yet b/c it
+        # wasn't known
+        self.assertEqual(
+            [f'octodns-version={__version__}', 'provider=dummy'], meta.values
+        )
+
+        # process the plan (Create)
+        plan = Plan(desired, self.zone, [Create(meta)], True)
+        got = proc.process_plan(plan, [], dummy_target)
+        self.assertTrue(got)
+
+        # process the plan (Update w/no changes)
+        plan = Plan(desired, self.zone, [Update(meta, meta)], True)
+        got = proc.process_plan(plan, [], dummy_target)
+        self.assertFalse(got)