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 new SelectelProvider class to support DNS v2 API. #40

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

dchudik
Copy link
Contributor

@dchudik dchudik commented Jan 22, 2024

Soon we are going to publicly announce that v2 DNS API is ready to use. API is available from internet, but there is no public docs, UI etc.
DNS v1 will be available and maintained but we gradually limit access to it until full shutdown.
So we would like to support both versions with this plugin.

This commit is all about that and also minor changes.

Changes

  • Move existing provider and related tests to separate directories v1
  • Rename public name from SelectelProvider to SelectelProviderLegacy
  • Add list_zones() method to support "*" for planning
  • Fix SSHFP parsing bug, caused by trailing dot in fingerprint which lead to constant re-update of record
  • Move version varible to separate file, since now it is utilized by two providers. Storing it in __init__.py causes cycling imports
  • Update script/release and setup.py to parse version from another location
  • Update readme.md with focus on new provider

New

  • Add new SelectelProvider class to support v2 API
  • Isolate API calls into separate class
  • Add tests for SelectelProvider

After these changes we also planning to bump plugin version to 1.0.0
All examples in readme are working(just change to unique zone-name). So if there is need to check it, they should suffice.

@ross
Copy link
Contributor

ross commented Jan 23, 2024

@dchudik
Copy link
Contributor Author

dchudik commented Jan 24, 2024

Replaced match-case to if-else in octodns_selectel/v2/dns_client.py for supporting python3.8/python3.9.

Renamed uuid to id in zone and rrset .
Renamed zone to zone_id in rrset.
API renamed these fields same.

@ross
Copy link
Contributor

ross commented Jan 25, 2024

Not really sure what it doesn't like now, I haven't done much w/types.

    def _request_all_entities(self, path, offset=0) -> list[int]:
E   TypeError: 'type' object is not subscriptable

https://github.com/octodns/octodns-selectel/actions/runs/7646141503/job/20870444134?pr=40#step:4:285

@dchudik
Copy link
Contributor Author

dchudik commented Jan 26, 2024

Removed type annotation for function.
Replace match-case to if-else in octodns_selectel/v2/mappings.py.

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.

So this is obviously a huge change and given that I don't have access to or deep knowledge of selecttel there's only so much that I can do in terms of the review details. I've skimmed over things and nothing that I saw jumped out. If you have a coworker or someone that can do a more focused look over things that's probably a good idea.

I'll give it until Tuesday next week or there about to allow for anyone to weight in before merging.

@Archirk
Copy link

Archirk commented Jan 30, 2024

So this is obviously a huge change and given that I don't have access to or deep knowledge of selecttel there's only so much that I can do in terms of the review details. I've skimmed over things and nothing that I saw jumped out. If you have a coworker or someone that can do a more focused look over things that's probably a good idea.

I'll give it until Tuesday next week or there about to allow for anyone to weight in before merging.

Hello,
This pull request has been done under my supervision, as his colleague at Selectel. It has been reviewed multiple times.
Core functionality has been tested and works fine.
After public launch we may eventually run in and do some bug fixes. But as for upgrading to major version - it safe to do.

@ross
Copy link
Contributor

ross commented Jan 30, 2024

This pull request has been done under my supervision, as his colleague at Selectel. It has been reviewed multiple times.
Core functionality has been tested and works fine.

Great. Thanks.

Just realized that a CHANGELOG.md entry wasn't included here. Definitely something that belongs there since the default behavior is going to be switching to the v2 api. Will approve & merge as soon as I see that come through.

I assume you all would like a release cut after that?

@Archirk
Copy link

Archirk commented Jan 31, 2024

Just realized that a CHANGELOG.md entry wasn't included here. Definitely something that belongs there since the default behavior is going to be switching to the v2 api. Will approve & merge as soon as I see that come through.

I assume you all would like a release cut after that?

If i remeber correctly, you usually merge/accept pull request first, than do commit with chagelog and version update and tag. So we didn't update changelog and version inside module intentionally.

I hope that changes and new paragraphs from PR are suitable to be just copy&paste into changelog.

@ross
Copy link
Contributor

ross commented Jan 31, 2024

If i remeber correctly, you usually merge/accept pull request first, than do commit with chagelog and version update and tag. So we didn't update changelog and version inside module intentionally.

Yeah. For releases the changelog date is finalized and the version number is bumped.

I generally try to have the changelog entries happen with the PRs that make the change so that blame on the CHANGELOG will take you to the PR that made the relevant change. It's for sure not 100% of the time, but it's a goal.

I hope that changes and new paragraphs from PR are suitable to be just copy&paste into changelog.

Looks like they probably are.

@dchudik
Copy link
Contributor Author

dchudik commented Feb 1, 2024

Updated changelog and added some fixes for idna zones.
I there anything else that is needed to be done you now can merge, bump and tag commit?

CHANGELOG.md Outdated Show resolved Hide resolved
@ross ross merged commit 9f2f38e into octodns:main Feb 1, 2024
7 checks passed
@ross
Copy link
Contributor

ross commented Feb 1, 2024

/cc #41 for release

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.

3 participants