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

Fix NSOne DNS Provider #1571

Merged
merged 1 commit into from Oct 22, 2021
Merged

Conversation

karantan
Copy link
Contributor

@karantan karantan commented Mar 17, 2021

Fix NSOne DNS Provider

Description

  • handle mx records
  • handle root domain

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@Kami
Copy link
Member

Kami commented Apr 15, 2021

Thanks for the contribution and sorry for the delay.

The change looks good to me, but it would be great if you could add corresponding unit tests.

@@ -206,7 +206,7 @@ def delete_record(self, record):

return response.status == httplib.OK

def create_record(self, name, zone, type, data, extra=None):
def create_record(self, name, zone, type_, data, extra=None):
Copy link
Member

Choose a reason for hiding this comment

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

type argument is part of the standard API (aka is the same for all the provider drivers) so we should use it here as well.

I know it's not ideal since it clashes with a built-int value, but not much we can do at this point (besides changing the standard API + updating all the drivers in the future).

Copy link
Member

Choose a reason for hiding this comment

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

Besides that, changes look good to me - please just add some tests when you get a chance.

@karantan
Copy link
Contributor Author

karantan commented May 3, 2021

hey, sorry for the delay. I was busy with other things ... anyway I added 3 tests and renamed type_ back to type. do we need to do anything else?

- handle mx records
- handle root domain
@karantan
Copy link
Contributor Author

@Kami gentle ping

Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

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

@karantan Sorry for a very late response.

Thanks for addressing the comments. Change LGTM 👍

@asfgit asfgit merged commit 4048faa into apache:trunk Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants