From 94bfb1e5072beec17f759b15344e3f0892d2955e Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sun, 21 Jan 2018 14:06:20 -0800 Subject: [PATCH] Switch populate to return exists, cleaner setup --- octodns/manager.py | 2 +- octodns/provider/base.py | 10 +++++++--- octodns/provider/plan.py | 4 ++-- octodns/source/base.py | 5 ++++- tests/test_octodns_plan.py | 4 ++-- tests/test_octodns_provider_azuredns.py | 6 +++--- tests/test_octodns_provider_base.py | 20 ++++++++++---------- tests/test_octodns_provider_cloudflare.py | 4 ++-- tests/test_octodns_provider_dyn.py | 2 +- tests/test_octodns_provider_googlecloud.py | 4 ++-- 10 files changed, 34 insertions(+), 27 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index bb2c921..3f46007 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -361,7 +361,7 @@ class Manager(object): plan = target.plan(zone) if plan is None: - plan = Plan(zone, zone, [], True) + plan = Plan(zone, zone, [], False) target.apply(plan) def validate_configs(self): diff --git a/octodns/provider/base.py b/octodns/provider/base.py index ddce7a6..6bfb16d 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -45,7 +45,12 @@ class BaseProvider(BaseSource): self.log.info('plan: desired=%s', desired.name) existing = Zone(desired.name, desired.sub_zones) - self.populate(existing, target=True, lenient=True) + exists = self.populate(existing, target=True, lenient=True) + if exists is None: + # If your code gets this warning see Source.populate for more + # information + self.log.warn('Provider %s used in target mode did not return ' + 'exists', self.id) # compute the changes at the zone/record level changes = existing.changes(desired, self) @@ -64,9 +69,8 @@ class BaseProvider(BaseSource): .join([str(c) for c in extra])) changes += extra - create = False if changes: - plan = Plan(existing, desired, changes, create, + plan = Plan(existing, desired, changes, exists, self.update_pcent_threshold, self.delete_pcent_threshold) self.log.info('plan: %s', plan) diff --git a/octodns/provider/plan.py b/octodns/provider/plan.py index 8bb72bd..10ab167 100644 --- a/octodns/provider/plan.py +++ b/octodns/provider/plan.py @@ -21,13 +21,13 @@ class Plan(object): MAX_SAFE_DELETE_PCENT = .3 MIN_EXISTING_RECORDS = 10 - def __init__(self, existing, desired, changes, create, + def __init__(self, existing, desired, changes, exists, update_pcent_threshold=MAX_SAFE_UPDATE_PCENT, delete_pcent_threshold=MAX_SAFE_DELETE_PCENT): self.existing = existing self.desired = desired self.changes = changes - self.create = create + self.exists = exists self.update_pcent_threshold = update_pcent_threshold self.delete_pcent_threshold = delete_pcent_threshold diff --git a/octodns/source/base.py b/octodns/source/base.py index 4ace09f..ee33619 100644 --- a/octodns/source/base.py +++ b/octodns/source/base.py @@ -22,7 +22,7 @@ class BaseSource(object): def populate(self, zone, target=False, lenient=False): ''' - Loads all zones the provider knows about + Loads all records the provider knows about for the provided zone When `target` is True the populate call is being made to load the current state of the provider. @@ -31,6 +31,9 @@ class BaseSource(object): do a "best effort" load of data. That will allow through some common, but not best practices stuff that we otherwise would reject. E.g. no trailing . or mising escapes for ;. + + When target is True (loading current state) this method should return + True if the zone exists or False if it does not. ''' raise NotImplementedError('Abstract base class, populate method ' 'missing') diff --git a/tests/test_octodns_plan.py b/tests/test_octodns_plan.py index 36dee68..91dd948 100644 --- a/tests/test_octodns_plan.py +++ b/tests/test_octodns_plan.py @@ -52,8 +52,8 @@ update = Update(existing, new) delete = Delete(new) changes = [create, delete, update] plans = [ - (simple, Plan(zone, zone, changes, False)), - (simple, Plan(zone, zone, changes, False)), + (simple, Plan(zone, zone, changes, True)), + (simple, Plan(zone, zone, changes, True)), ] diff --git a/tests/test_octodns_provider_azuredns.py b/tests/test_octodns_provider_azuredns.py index f44d368..adf394b 100644 --- a/tests/test_octodns_provider_azuredns.py +++ b/tests/test_octodns_provider_azuredns.py @@ -339,9 +339,9 @@ class TestAzureDnsProvider(TestCase): deletes.append(Delete(i)) self.assertEquals(13, provider.apply(Plan(None, zone, - changes, False))) + changes, True))) self.assertEquals(13, provider.apply(Plan(zone, zone, - deletes, False))) + deletes, True))) def test_create_zone(self): provider = self._get_provider() @@ -357,7 +357,7 @@ class TestAzureDnsProvider(TestCase): _get.side_effect = CloudError(Mock(status=404), err_msg) self.assertEquals(13, provider.apply(Plan(None, desired, changes, - False))) + True))) def test_check_zone_no_create(self): provider = self._get_provider() diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index a0ec74e..3ebf250 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -153,7 +153,7 @@ class TestBaseProvider(TestCase): def test_safe_none(self): # No changes is safe - Plan(None, None, [], False).raise_if_unsafe() + Plan(None, None, [], True).raise_if_unsafe() def test_safe_creates(self): # Creates are safe when existing records is under MIN_EXISTING_RECORDS @@ -164,7 +164,7 @@ class TestBaseProvider(TestCase): 'type': 'A', 'value': '1.2.3.4', }) - Plan(zone, zone, [Create(record) for i in range(10)], False) \ + Plan(zone, zone, [Create(record) for i in range(10)], True) \ .raise_if_unsafe() def test_safe_min_existing_creates(self): @@ -184,7 +184,7 @@ class TestBaseProvider(TestCase): 'value': '2.3.4.5' })) - Plan(zone, zone, [Create(record) for i in range(10)], False) \ + Plan(zone, zone, [Create(record) for i in range(10)], True) \ .raise_if_unsafe() def test_safe_no_existing(self): @@ -197,7 +197,7 @@ class TestBaseProvider(TestCase): }) updates = [Update(record, record), Update(record, record)] - Plan(zone, zone, updates, False).raise_if_unsafe() + Plan(zone, zone, updates, True).raise_if_unsafe() def test_safe_updates_min_existing(self): # MAX_SAFE_UPDATE_PCENT+1 fails when more @@ -221,7 +221,7 @@ class TestBaseProvider(TestCase): Plan.MAX_SAFE_UPDATE_PCENT) + 1)] with self.assertRaises(UnsafePlan) as ctx: - Plan(zone, zone, changes, False).raise_if_unsafe() + Plan(zone, zone, changes, True).raise_if_unsafe() self.assertTrue('Too many updates' in ctx.exception.message) @@ -245,7 +245,7 @@ class TestBaseProvider(TestCase): for i in range(int(Plan.MIN_EXISTING_RECORDS * Plan.MAX_SAFE_UPDATE_PCENT))] - Plan(zone, zone, changes, False).raise_if_unsafe() + Plan(zone, zone, changes, True).raise_if_unsafe() def test_safe_deletes_min_existing(self): # MAX_SAFE_DELETE_PCENT+1 fails when more @@ -269,7 +269,7 @@ class TestBaseProvider(TestCase): Plan.MAX_SAFE_DELETE_PCENT) + 1)] with self.assertRaises(UnsafePlan) as ctx: - Plan(zone, zone, changes, False).raise_if_unsafe() + Plan(zone, zone, changes, True).raise_if_unsafe() self.assertTrue('Too many deletes' in ctx.exception.message) @@ -293,7 +293,7 @@ class TestBaseProvider(TestCase): for i in range(int(Plan.MIN_EXISTING_RECORDS * Plan.MAX_SAFE_DELETE_PCENT))] - Plan(zone, zone, changes, False).raise_if_unsafe() + Plan(zone, zone, changes, True).raise_if_unsafe() def test_safe_updates_min_existing_override(self): safe_pcent = .4 @@ -318,7 +318,7 @@ class TestBaseProvider(TestCase): safe_pcent) + 1)] with self.assertRaises(UnsafePlan) as ctx: - Plan(zone, zone, changes, False, + Plan(zone, zone, changes, True, update_pcent_threshold=safe_pcent).raise_if_unsafe() self.assertTrue('Too many updates' in ctx.exception.message) @@ -346,7 +346,7 @@ class TestBaseProvider(TestCase): safe_pcent) + 1)] with self.assertRaises(UnsafePlan) as ctx: - Plan(zone, zone, changes, False, + Plan(zone, zone, changes, True, delete_pcent_threshold=safe_pcent).raise_if_unsafe() self.assertTrue('Too many deletes' in ctx.exception.message) diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 7925b94..2977d26 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -347,7 +347,7 @@ class TestCloudflareProvider(TestCase): 'values': ['2.2.2.2', '3.3.3.3', '4.4.4.4'], }) change = Update(existing, new) - plan = Plan(zone, zone, [change], False) + plan = Plan(zone, zone, [change], True) provider._apply(plan) provider._request.assert_has_calls([ @@ -432,7 +432,7 @@ class TestCloudflareProvider(TestCase): 'value': 'ns2.foo.bar.', }) change = Update(existing, new) - plan = Plan(zone, zone, [change], False) + plan = Plan(zone, zone, [change], True) provider._apply(plan) provider._request.assert_has_calls([ diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index 9e06a6b..0956477 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -913,7 +913,7 @@ class TestDynProviderGeo(TestCase): Delete(geo), Delete(regular), ] - plan = Plan(None, desired, changes, False) + plan = Plan(None, desired, changes, True) provider._apply(plan) mock.assert_has_calls([ call('/Zone/unit.tests/', 'GET', {}), diff --git a/tests/test_octodns_provider_googlecloud.py b/tests/test_octodns_provider_googlecloud.py index 7060355..dacc369 100644 --- a/tests/test_octodns_provider_googlecloud.py +++ b/tests/test_octodns_provider_googlecloud.py @@ -264,7 +264,7 @@ class TestGoogleCloudProvider(TestCase): existing=[update_existing_r, delete_r], desired=desired, changes=changes, - create=False + exists=True )) calls_mock = gcloud_zone_mock.changes.return_value @@ -297,7 +297,7 @@ class TestGoogleCloudProvider(TestCase): existing=[update_existing_r, delete_r], desired=desired, changes=changes, - create=False + exists=True )) unsupported_change = Mock()