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.

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

Conversation

rlaakkol
Copy link
Contributor

Enable logging zone name server information on apply. This makes managing new zones easier, as otherwise one has to go look up the name server information for the zone separately in order to configure the domain registrar side records.

The name server information is already present in the responses that the zone method receives, so adding it is easy enough. Downside is that a nested dict structure is required, which makes lookups a bit more complex.

The nested dict part might be a bit controversial, and I'm by no means a python dev, so any pointers on how to do this in a more elegant way I will gladly take!

Enable logging zone name server information on apply. This makes
managing new zones easier, as otherwise one has to go look up the name
server information for the zone separately in order to configure the domain
registrar side records.

The name server information is already present in the responses that the
zone method receives, so adding it is easy enough. Downside is that a
nested dict structure is required, which makes lookups a bit more
complex.
Copy link
Contributor

@ross ross left a comment

Choose a reason for hiding this comment

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

Couple comments for 💭 and discussion inline. Overall seems like a good idea.

octodns_cloudflare/__init__.py Outdated Show resolved Hide resolved
}
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.

Copy link
Contributor

@ross ross left a comment

Choose a reason for hiding this comment

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

Looks good. One question inline. Will wait on the conclusion of that thread before merging.

}
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.

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

octodns_cloudflare/__init__.py Outdated Show resolved Hide resolved
@ross ross merged commit ad474fb into octodns:main Feb 4, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants