From ce741fc887e2481e9b566e029a88cb860953fa6b Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 29 Nov 2023 13:22:41 -0800 Subject: [PATCH 1/7] Ignore deprecation warnings during tests --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 74c48d7..f54dbb0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -13,4 +13,4 @@ filterwarnings = [ 'error', 'ignore:.*DEPRECATED.*2.0', ] -pythonpath = "." \ No newline at end of file +pythonpath = "." From 6f6cb798541ce47051d4747162e01c3d290b42e8 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 16:27:02 -0800 Subject: [PATCH 2/7] Fix bug with Record.copy when values is an empty list [] --- CHANGELOG.md | 1 + octodns/record/base.py | 5 ++++- tests/test_octodns_record.py | 18 ++++++++++++++---- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc3fef1..8e25a7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## v1.?.0 - 2023-??-?? - * Record.lenient property added similar to other common/standard _octodns data +* Fix bug with Record.copy when values is an empty list [] ## v1.3.0 - 2023-11-14 - New and improved processors diff --git a/octodns/record/base.py b/octodns/record/base.py index 6f83578..d203cea 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -296,7 +296,10 @@ class ValuesMixin(object): try: values = data['values'] except KeyError: - values = [data['value']] + try: + values = [data['value']] + except KeyError: + values = [] self.values = sorted(self._value_type.process(values)) def changes(self, other, target): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 66f0b9f..0fe3d57 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -338,6 +338,17 @@ class TestRecord(TestCase): del b._octodns['key']['second'] self.assertNotEqual(a.data, b.data) + def test_record_copy_with_no_values(self): + txt = Record.new( + self.zone, + 'txt', + {'ttl': 45, 'type': 'TXT', 'values': []}, + lenient=True, + ) + + dup = txt.copy() + self.assertEqual(txt.values, dup.values) + def test_change(self): existing = Record.new( self.zone, 'txt', {'ttl': 44, 'type': 'TXT', 'value': 'some text'} @@ -631,10 +642,9 @@ class TestRecordValidation(TestCase): lenient=True, ) - # __init__ may still blow up, even if validation is lenient - with self.assertRaises(KeyError) as ctx: - Record.new(self.zone, 'www', {'type': 'A', 'ttl': -1}, lenient=True) - self.assertEqual(('value',), ctx.exception.args) + # empty values is allowed with lenient + r = Record.new(self.zone, 'www', {'type': 'A', 'ttl': -1}, lenient=True) + self.assertEqual([], r.values) # no exception if we're in lenient mode from config Record.new( From 1a5c9a2c5e612c86c246724cb88ec51de54a13f1 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 29 Nov 2023 14:03:22 -0800 Subject: [PATCH 3/7] Flip ValuesMixin._values logic to handle/have key in all cases --- octodns/record/base.py | 14 +++++++------- tests/test_octodns_record.py | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/octodns/record/base.py b/octodns/record/base.py index d203cea..5d5d0af 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -309,16 +309,16 @@ class ValuesMixin(object): def _data(self): ret = super()._data() - if len(self.values) > 1: - values = [getattr(v, 'data', v) for v in self.values if v] - if len(values) > 1: - ret['values'] = values - elif len(values) == 1: - ret['value'] = values[0] - elif len(self.values) == 1: + if len(self.values) == 1: v = self.values[0] if v: ret['value'] = getattr(v, 'data', v) + else: + values = [getattr(v, 'data', v) for v in self.values if v] + if len(values) == 1: + ret['value'] = values[0] + else: + ret['values'] = values return ret diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 0fe3d57..4d52b71 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -197,19 +197,19 @@ class TestRecord(TestCase): ) def test_values_mixin_data(self): - # no values, no value or values in data + # empty values -> empty values in data a = ARecord(self.zone, '', {'type': 'A', 'ttl': 600, 'values': []}) - self.assertNotIn('values', a.data) + self.assertEqual([], a.data['values']) # empty value, no value or values in data b = ARecord(self.zone, '', {'type': 'A', 'ttl': 600, 'values': ['']}) self.assertNotIn('value', b.data) - # empty/None values, no value or values in data + # empty/None values -> empty values in data c = ARecord( self.zone, '', {'type': 'A', 'ttl': 600, 'values': ['', None]} ) - self.assertNotIn('values', c.data) + self.assertEqual([], a.data['values']) # empty/None values and valid, value in data c = ARecord( From 5568dc6be15c83773e95f1bd6f0ceaf2417e0190 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 15:42:53 -0800 Subject: [PATCH 4/7] Add Processor.process_source_and_target_zones --- CHANGELOG.md | 2 ++ octodns/processor/base.py | 29 +++++++++++++++++++++- octodns/provider/base.py | 5 ++++ tests/test_octodns_manager.py | 46 ++++++++++++++++++++++++++++++++++- 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc3fef1..1d32bd6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## v1.?.0 - 2023-??-?? - * Record.lenient property added similar to other common/standard _octodns data +* Processor.process_source_and_target_zones added to support modifying both the + desired and/or existing zones just prior to computing changes. ## v1.3.0 - 2023-11-14 - New and improved processors diff --git a/octodns/processor/base.py b/octodns/processor/base.py index f0890e0..eb584d5 100644 --- a/octodns/processor/base.py +++ b/octodns/processor/base.py @@ -37,7 +37,7 @@ class BaseProcessor(object): computed between `existing` and `desired`. This provides an opportunity to modify the `existing` `Zone`. - - Will see `existing` after any modifrications done by processors + - Will see `existing` after any modifications done by processors configured to run before this one. - May modify `existing` directly. - Must return `existing` which will normally be the `existing` param. @@ -48,6 +48,33 @@ class BaseProcessor(object): ''' return existing + def process_source_and_target_zones(self, desired, existing, target): + ''' + Called just prior to computing changes for `target` between `desired` + and `existing`. Provides an opportunity for the processor to modify + either the desired or existing `Zone`s that will be used to compute the + changes and create the initial plan. + + - Will see `desired` after any modifications done by + `Provider._process_desired_zone` and all processors via + `Processor.process_source_zone` + - Will see `existing` after any modifications done by all processors + via `Processor.process_target_zone` + - Will see both `desired` and `existing` after any modifications done by + any processors configured to run before this one via + `Processor.process_source_and_target_zones`. + - May modify `desired` directly. + - Must return `desired` which will normally be the `desired` param. + - May modify `existing` directly. + - Must return `existing` which will normally be the `existing` param. + - Must not modify records directly, `record.copy` should be called, + the results of which can be modified, and then `Zone.add_record` may + be used with `replace=True`. + - May call `Zone.remove_record` to remove records from `desired`. + - May call `Zone.remove_record` to remove records from `existing`. + ''' + return desired, existing + def process_plan(self, plan, sources, target): ''' Called after the planning phase has completed. Provides an opportunity diff --git a/octodns/provider/base.py b/octodns/provider/base.py index e1e2234..9696c57 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -243,6 +243,11 @@ class BaseProvider(BaseSource): for processor in processors: existing = processor.process_target_zone(existing, target=self) + for processor in processors: + desired, existing = processor.process_source_and_target_zones( + desired, existing, self + ) + # compute the changes at the zone/record level changes = existing.changes(desired, self) diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 2bec87e..72f0ec5 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -25,7 +25,7 @@ from octodns.manager import ( _AggregateTarget, ) from octodns.processor.base import BaseProcessor -from octodns.record import Create, Delete, Record +from octodns.record import Create, Delete, Record, Update from octodns.yaml import safe_load from octodns.zone import Zone @@ -723,6 +723,50 @@ class TestManager(TestCase): # We got a delete for the thing added to the existing state (target) self.assertIsInstance(plans[0][1].changes[0], Delete) + # source & target + record2 = Record.new( + zone, 'a2', {'ttl': 31, 'type': 'A', 'value': '1.2.3.4'} + ) + record3 = Record.new( + zone, 'a3', {'ttl': 32, 'type': 'A', 'value': '1.2.3.4'} + ) + + class MockProcessor(BaseProcessor): + def process_source_and_target_zones( + self, desired, existing, target + ): + # add something to desired + desired.add_record(record2) + # add something to existing + existing.add_record(record3) + # add something to both, but with a modification + desired.add_record(record) + mod = record.copy() + mod.ttl += 1 + existing.add_record(mod) + return desired, existing + + mock = MockProcessor('mock') + plans, zone = manager._populate_and_plan( + 'unit.tests.', [mock], [], targets + ) + # we should see a plan + self.assertTrue(plans) + plan = plans[0][1] + # it shoudl have a create, an update, and a delete + self.assertEqual( + 'a', + next(c.record.name for c in plan.changes if isinstance(c, Update)), + ) + self.assertEqual( + 'a2', + next(c.record.name for c in plan.changes if isinstance(c, Create)), + ) + self.assertEqual( + 'a3', + next(c.record.name for c in plan.changes if isinstance(c, Delete)), + ) + # muck with plans class MockProcessor(BaseProcessor): def process_target_zone(self, zone, target): From cb09b590e7dfab789d781a38207008d9a6efec3a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 29 Nov 2023 14:21:14 -0800 Subject: [PATCH 5/7] ValueMixin._data always includes 'value' --- octodns/record/base.py | 3 +-- tests/test_octodns_record.py | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/octodns/record/base.py b/octodns/record/base.py index 5d5d0af..443e7ba 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -374,8 +374,7 @@ class ValueMixin(object): def _data(self): ret = super()._data() - if self.value: - ret['value'] = getattr(self.value, 'data', self.value) + ret['value'] = getattr(self.value, 'data', self.value) return ret @property diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 4d52b71..86f92e5 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -225,13 +225,13 @@ class TestRecord(TestCase): a = AliasRecord( self.zone, '', {'type': 'ALIAS', 'ttl': 600, 'value': None} ) - self.assertNotIn('value', a.data) + self.assertIsNone(a.data['value']) # unspecified value, no value in data a = AliasRecord( self.zone, '', {'type': 'ALIAS', 'ttl': 600, 'value': ''} ) - self.assertNotIn('value', a.data) + self.assertIsNone(a.data['value']) def test_record_new(self): txt = Record.new( @@ -349,6 +349,16 @@ class TestRecord(TestCase): dup = txt.copy() self.assertEqual(txt.values, dup.values) + cname = Record.new( + self.zone, + 'cname', + {'ttl': 45, 'type': 'CNAME', 'value': ''}, + lenient=True, + ) + + dup = cname.copy() + self.assertEqual(cname.value, dup.value) + def test_change(self): existing = Record.new( self.zone, 'txt', {'ttl': 44, 'type': 'TXT', 'value': 'some text'} From 0d1fcb4a3973372d64fcedc092edbf40ef1f71bd Mon Sep 17 00:00:00 2001 From: "Janik H." Date: Thu, 30 Nov 2023 20:33:03 +0100 Subject: [PATCH 6/7] Update README.md --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4c5cac0..f69f17c 100644 --- a/README.md +++ b/README.md @@ -313,8 +313,8 @@ Similar to providers, but can only serve to populate records into a zone, cannot | Source | Record Support | Dynamic | Notes | |--|--|--|--| | [EnvVarSource](/octodns/source/envvar.py) | TXT | No | read-only environment variable injection | -| [AxfrSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, LOC, MX, NS, PTR, SPF, SRV, TXT | No | read-only | -| [ZoneFileSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, MX, NS, PTR, SPF, SRV, TXT | No | read-only | +| [AxfrSource](https://github.com/octodns/octodns-bind/) | A, AAAA, CAA, CNAME, LOC, MX, NS, PTR, SPF, SRV, TXT | No | read-only | +| [ZoneFileSource](https://github.com/octodns/octodns-bind/) | A, AAAA, CAA, CNAME, MX, NS, PTR, SPF, SRV, TXT | No | read-only | | [TinyDnsFileSource](/octodns/source/tinydns.py) | A, CNAME, MX, NS, PTR | No | read-only | ### Notes From 523a188e1decc9a60a114c6d85fad4f3d62e9dfd Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 1 Dec 2023 10:46:58 -0800 Subject: [PATCH 7/7] explicit test for values and value --- tests/test_octodns_record.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 86f92e5..c85341f 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -251,6 +251,20 @@ class TestRecord(TestCase): Record.new(self.zone, 'unknown', {'type': 'XXX'}) self.assertTrue('Unknown record type' in str(ctx.exception)) + def test_record_new_with_values_and_value(self): + a = Record.new( + self.zone, + 'a', + { + 'ttl': 44, + 'type': 'A', + 'value': '1.2.3.4', + 'values': ['2.3.4.5', '3.4.5.6'], + }, + ) + # values is preferred over value when both exist + self.assertEqual(['2.3.4.5', '3.4.5.6'], a.values) + def test_record_copy(self): a = Record.new( self.zone, 'a', {'ttl': 44, 'type': 'A', 'value': '1.2.3.4'}