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

feat: Support root nameserver and additional record types #40

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

maikelpoot
Copy link
Contributor

@maikelpoot maikelpoot commented Sep 27, 2024

This PR will add support for all additional record types transip supports at the moment:

  • Root NS
  • ALIAS
  • DS
  • NAPTR
  • TLSA

@maikelpoot
Copy link
Contributor Author

Ideally the caveats would be catched/fixed in the planning/validating phase but couldn't find any documentation or examples how to.

@ross
Copy link
Contributor

ross commented Sep 27, 2024

Ideally the caveats would be catched/fixed in the planning/validating phase but couldn't find any documentation or examples how to.

_process_desired_zone is the place to do that sort of checking, it's requirements are in the docstring:

https://github.com/octodns/octodns/blob/9f73103300927733c691bf765866ec5c6f2e5784/octodns/provider/base.py#L35-L52

You can search the org for what various providers are doing/checking in that function:

https://github.com/search?q=org%3Aoctodns%20_process_desired_zone&type=code

The azure provider looks like it has something that's fairly close to the root caveats:

https://github.com/octodns/octodns-azure/blob/374f95986d884dc603ba0dbe55226c2b6595cf6d/octodns_azure/__init__.py#L1343

As for the TTL bit I'm not sure what makes sense. It could always warn if there's a non-matching value and just go ahead with that value or it could do the standard supports_warn_or_except behavior.

@maikelpoot
Copy link
Contributor Author

_process_desired_zone is indeed what i was searching for.
Refactored the code to validate the root nameserver in _process_desired_zone

@maikelpoot maikelpoot marked this pull request as draft October 3, 2024 05:15
@maikelpoot
Copy link
Contributor Author

Changed it to draft for now, The opt-in for NS is a pain in .... when dealing with larger sets of domains in the config.

Trying to refactor to something like the googlecloud provider:

WARNING GoogleCloudProvider[google_public] root NS record supported, but no record is configured for example.com.
 INFO  GoogleCloudProvider[google_public] root NS record in existing, but not supported or not configured; ignoring it

@ross
Copy link
Contributor

ross commented Oct 3, 2024

Trying to refactor to something like the googlecloud provider:

IIRC that's just using the default/built-in root handling of the base class.

https://github.com/octodns/octodns/blob/9f73103300927733c691bf765866ec5c6f2e5784/octodns/provider/base.py#L152-L166

Basic functionality of octodns allready takes care of non-existing root-ns records in source.
Also gave all api-calls a dedicated try block so more specific exceptions can be raised (and tested)
@maikelpoot maikelpoot marked this pull request as ready for review October 10, 2024 07:43
@maikelpoot
Copy link
Contributor Author

@ross Thanks for the info.
Removed the opt-in code, updated all the tests and tested the code with some real-world config.
So i think this one is ready for review.

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.

Overall looks good. Few minor things inline and it should be ready to 🚢

Thanks.

octodns_transip/__init__.py Outdated Show resolved Hide resolved
octodns_transip/__init__.py Outdated Show resolved Hide resolved
octodns_transip/__init__.py Outdated Show resolved Hide resolved
octodns_transip/__init__.py Outdated Show resolved Hide resolved
octodns_transip/__init__.py Outdated Show resolved Hide resolved
octodns_transip/__init__.py Outdated Show resolved Hide resolved
@maikelpoot
Copy link
Contributor Author

Thought i tried that before without success, but maybe did something else wrong then.

Removed the usage of .data for both the record= and the if change.data['name'] == '' and change.data['record_type'] == 'NS': using the record from "new" to check.

@ross ross merged commit e9bd96f into octodns:main Oct 14, 2024
7 checks passed
@ross
Copy link
Contributor

ross commented Oct 14, 2024

Thanks.

@maikelpoot
Copy link
Contributor Author

@ross Do you have an idea when the next release of the provider will be (and/or when it will be included within the docker containers, github action etc etc)

@ross
Copy link
Contributor

ross commented Oct 17, 2024

/cc #42

@maikelpoot maikelpoot deleted the feature/more-record-types branch October 18, 2024 11:12
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