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

adding dns support for dnspod provider #787

Closed
wants to merge 7 commits into from

Conversation

piratimir
Copy link
Contributor

@piratimir piratimir commented May 15, 2016

DNS support for DNSPod provider

Description

I coded the dns driver for DNSPod on libcloud/dns/drivers/dnspod.py module. In order for this driver to work the libcloud/common/dnspod.py module is needed. The unitttests for this driver are coded in the libcloud/test/dns/test_dnspod.py module.

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 May 15, 2016

Great, thanks.

Is the PR already for a review, or are you still working on it?

@piratimir
Copy link
Contributor Author

@Kami the pr is ready for review :)

url, body, headers):
body = self.fixtures.load('delete_record_record_does_not_exist.json')

return httplib.OK, body, {}, httplib.responses[httplib.OK]
Copy link
Member

Choose a reason for hiding this comment

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

Just curious - does API indeed return 200 status code for all the different responses?

@Kami
Copy link
Member

Kami commented May 16, 2016

Thanks 👍

I've added some in-line comments.

@Kami
Copy link
Member

Kami commented May 21, 2016

@jetbird Did you push your latest changes?

Since I don't see any new commits (license header is still missing, etc.).

@piratimir
Copy link
Contributor Author

@Kami done :)

creates, removes uneeded default headers, calling response.object instead
of response.parse_body
@asfgit asfgit closed this in 707a725 May 25, 2016
asfgit pushed a commit that referenced this pull request May 25, 2016
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
asfgit pushed a commit that referenced this pull request May 25, 2016
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
asfgit pushed a commit that referenced this pull request May 25, 2016
…ython versions

Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
asfgit pushed a commit that referenced this pull request May 25, 2016
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
asfgit pushed a commit that referenced this pull request May 25, 2016
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
asfgit pushed a commit that referenced this pull request May 25, 2016
…m creates, removes uneeded default headers, calling response.object instead of response.parse_body

Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
@Kami
Copy link
Member

Kami commented May 25, 2016

@jetbird Thanks.

I've made some changes and fixes (6594a46, defff8a) and merged patch into trunk.

As you can see in the second commit (defff8a), _make_request method didn't actually do anything with the method argument (a bug which existing tests don't catch). That was masked by (ab)using **kwargs - that's a good example of why **kwargs should be avoided in scenarios like that and only used when there is a valid need for it.

@Kami
Copy link
Member

Kami commented May 25, 2016

@jetbird Can you please test latest changes against the live provider API?

@piratimir
Copy link
Contributor Author

Thanks for explaining the fixes :) Of course, I will test it now.

@piratimir
Copy link
Contributor Author

piratimir commented May 25, 2016

@Kami Tested. It works fine for me :)

@Kami
Copy link
Member

Kami commented May 25, 2016

@jetbird Great 👍

It would also be good if you can add some basic docs for the driver and contact the provider (tall them about Libcloud, driver, etc. and ask them if it's possible to mention it somewhere on the page / docs, etc.).

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.

None yet

2 participants