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 support for Name.com #507

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Jamim
Copy link

@Jamim Jamim commented May 14, 2020

Hello,

I'd like to propose adding support for the Name.com domain name registrar.
Name.com API docs: https://www.name.com/api-docs/DNS

Hope you might find these changes useful.

Best regards!

@Jamim
Copy link
Author

Jamim commented May 28, 2020

Hello @AnalogJ,
Would you mind reviewing this PR?
Thank you!

@Jamim
Copy link
Author

Jamim commented Jun 3, 2020

Hello @adferrand,
Could you please review this PR?
Thanks!

@adferrand adferrand self-assigned this Jun 3, 2020
@adferrand
Copy link
Collaborator

Hello @Jamim, I put your PR on my backlog, I should be able to review it by Friday.

@Jamim
Copy link
Author

Jamim commented Jun 3, 2020

Sounds great, thank you!

Copy link
Collaborator

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Jamim. I opened some remarks that are mainly details that are not impacting the behavior. It is a great job!

I would like to thanks you specifically for adding that amount of new integration tests for your provider on top of the standard ones, and I am seriously thinking in adding some of them, if not all, as a new standard test suite for all new providers!

records = (record for record in records
if record['content'] == content)

if not isinstance(records, list):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a specific reason to use comprehension tuples above which then need to cast back tuples into list here ? From what I see, you could use comprehension lists directly ([record for record in records if ...]) above, and avoid this cast.

if not records:
LOGGER.warning('delete_record: there is no record to delete')
return None
record_ids = tuple(record['id'] for record in records)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A comprehension list would be a better fit here, since it will be iterated after (record_ids = [record['id'] for record in records]).

return None
record_ids = tuple(record['id'] for record in records)
else:
record_ids = (identifier,)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here: records_ids = [identifier,]

'delete_record: record %s has been deleted', record_id
)

return record_ids if len(record_ids) > 1 else record_ids[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our specification about delete is to return True if the delete was successful.

return self._get(url)

def _request(self, action='GET', url='/', data=None, query_params=None):
response = self.session.request(method=action,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it strictly required to use an HTTP session with pre-authentication? I would prefer if possible to use plain request each time because I saw from my experience that HTTP sessions are raising their own problems. However if it speeds up the requests, I am ok with it if it works for Name.com.

"""TestCase for Name.com"""

# I don't think we really need some docstrings here.
# pylint: disable=missing-function-docstring
Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, most of the time I even put this pylint disable directive at the top of the test module ;)

# Provider.authenticate() #
###########################
@_vcr_integration_test
def test_provider_authenticate(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you rename this test to not shadow the existing one in integration_test?

@adferrand
Copy link
Collaborator

Hello @Jamim! Are you willing to continue on your PR and answer to my questions? Thanks in advance!

@Jamim
Copy link
Author

Jamim commented Jul 18, 2020

Hello @adferrand,
I'll try to devote time to working on this PR in the next few days.
P.S. Sorry for the delay.

@adferrand
Copy link
Collaborator

Re @Jamim, quick reminder if you find the time to finish the PR :)

@Jamim Jamim requested a review from AnalogJ as a code owner March 17, 2021 20:43
@giuseongit
Copy link
Contributor

I'll try to bring to merge this pr, I really need the support for name.com.

@adferrand can I rebase @Jamim 's code onto master or do you prefer to merge master into this branch again?

@adferrand
Copy link
Collaborator

Hello @giuseongit, it is really up to you. I am a big fan of the strategy "stop rebasing once the branch becomes public", because it helps for the review by keeping track of all changes, but if you want to take over this PR on a clean state, this is also a viable strategy. We have the current PR for the historical review.

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