Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to log name server information on apply #122

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Add option to log name server information on apply #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbd89b4
Add option to log name server information on apply
rlaakkol Jan 31, 2025
c1ae965
Merge remote-tracking branch 'origin/main' into output-nameservers
rlaakkol Feb 3, 2025
0ca3b2a
Remove option and always log name servers
rlaakkol Feb 3, 2025
8f7a7b5
Merge branch 'main' into output-nameservers
ross Feb 3, 2025
a555ed9
Remove useless get
rlaakkol Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions octodns_cloudflare/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,15 @@ def zones(self):
else:
page = None

self._zones = IdnaDict({f'{z["name"]}.': z['id'] for z in zones})
self._zones = IdnaDict(
{
f'{z["name"]}.': {
'id': z['id'],
'name_servers': z['name_servers'],
}
for z in zones
}
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd rather avoid the nested dict and having to change all the places _zones is used you could instead go with a seperate dict, self._namerservers that just stores that information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll try that out and see how it looks/feels! I suppose there's nothing that wrong with nested dicts in general, just feels a bit icky to do these self.zones.get(zone.name, {}).get('id', False) tricks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to leave the nested dict after all, as now it's pretty simple to add new potential information from the zones to the dict. #123 perphaps wants to utilize the same approach, so it's perhaps nice to group all the non-record zone info under the same dict instead of always adding a new one, or what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#123 has been superseded by #127. Same should apply to it though.


return self._zones

Expand Down Expand Up @@ -485,7 +493,7 @@ def _data_for_SSHFP(self, _type, records):

def zone_records(self, zone):
if zone.name not in self._zone_records:
zone_id = self.zones.get(zone.name, False)
zone_id = self.zones.get(zone.name, {}).get('id', False)
if not zone_id:
return []

Expand Down Expand Up @@ -1033,7 +1041,7 @@ def _gen_key(self, data):

def _apply_Create(self, change):
new = change.new
zone_id = self.zones[new.zone.name]
zone_id = self.zones[new.zone.name]['id']
if new._type == 'URLFWD':
path = f'/zones/{zone_id}/pagerules'
else:
Expand All @@ -1043,7 +1051,7 @@ def _apply_Create(self, change):

def _apply_Update(self, change):
zone = change.new.zone
zone_id = self.zones[zone.name]
zone_id = self.zones[zone.name]['id']
hostname = zone.hostname_from_fqdn(change.new.fqdn[:-1])
_type = change.new._type

Expand Down Expand Up @@ -1176,7 +1184,7 @@ def _apply_Delete(self, change):
existing_name = existing.fqdn[:-1]
# Make sure to map ALIAS to CNAME when looking for the target to delete
existing_type = 'CNAME' if existing._type == 'ALIAS' else existing._type
zone_id = self.zones[existing.zone.name]
zone_id = self.zones.get(existing.zone.name, {}).get('id', False)
for record in self.zone_records(existing.zone):
if 'targets' in record and self.pagerules:
uri = record['targets'][0]['constraint']['value']
Expand Down Expand Up @@ -1224,9 +1232,17 @@ def _apply(self, plan):
data['account'] = {'id': self.account_id}
resp = self._try_request('POST', '/zones', data=data)
zone_id = resp['result']['id']
self.zones[name] = zone_id
name_servers = resp['result']['name_servers']
self.zones[name] = {'id': zone_id, 'name_servers': name_servers}
self._zone_records[name] = {}

self.log.info(
'zone %s (id %s) name servers: %s',
name,
self.zones[name]['id'],
self.zones[name]['name_servers'],
)

# Force the operation order to be Delete() -> Create() -> Update()
# This will help avoid problems in updating a CNAME record into an
# A record and vice-versa
Expand Down
10 changes: 5 additions & 5 deletions tests/test_octodns_provider_cloudflare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ def test_apply(self):

provider._request.side_effect = [
self.empty, # no zones
{'result': {'id': 42}}, # zone create
{'result': {'id': 42, 'name_servers': ['foo']}}, # zone create
] + [
None
] * 34 # individual record creates
Expand Down Expand Up @@ -595,7 +595,7 @@ def test_apply(self):

provider._request.side_effect = [
self.empty, # no zones
{'result': {'id': 42}}, # zone create
{'result': {'id': 42, 'name_servers': ['foo']}}, # zone create
] + [
None
] * 34 # individual record creates
Expand Down Expand Up @@ -715,7 +715,7 @@ def test_update_add_swap(self):
provider._request.side_effect = [
CloudflareRateLimitError('{}'),
self.empty, # no zones
{'result': {'id': 42}}, # zone create
{'result': {'id': 42, 'name_servers': ['foo']}}, # zone create
None,
None,
None,
Expand Down Expand Up @@ -888,7 +888,7 @@ def test_update_delete(self):
provider._request.side_effect = [
CloudflareRateLimitError('{}'),
self.empty, # no zones
{'result': {'id': 42}}, # zone create
{'result': {'id': 42, 'name_servers': ['foo']}}, # zone create
None,
None,
None,
Expand Down Expand Up @@ -1024,7 +1024,7 @@ def test_pagerules(self):
# Set things up to preexist/mock as necessary
zone = Zone('unit.tests.', [])
# Stuff a fake zone id in place
provider._zones = {zone.name: '42'}
provider._zones = {zone.name: {'id': '42', 'name_servers': ['foo']}}
provider._request = Mock()
side_effect = [
{
Expand Down