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
base: trunk
from

Conversation

Projects
None yet
2 participants
@jetbird
Contributor

jetbird 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

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 15, 2016

Member

Great, thanks.

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

Member

Kami commented May 15, 2016

Great, thanks.

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

@jetbird

This comment has been minimized.

Show comment
Hide comment
@jetbird

jetbird May 15, 2016

Contributor

@Kami the pr is ready for review :)

Contributor

jetbird commented May 15, 2016

@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]

This comment has been minimized.

@Kami

Kami May 16, 2016

Member

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

@Kami

Kami May 16, 2016

Member

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

@@ -0,0 +1,65 @@
from libcloud.common.base import ConnectionKey, JsonResponse

This comment has been minimized.

@Kami

Kami May 16, 2016

Member

Please add a missing Apache license header.

@Kami

Kami May 16, 2016

Member

Please add a missing Apache license header.

Show outdated Hide outdated libcloud/common/dnspod.py
def add_default_headers(self, headers):
headers['Content-Type'] = 'application/x-www-form-urlencoded'
headers['Accept'] = 'text/json'
headers['User-Agent'] = \

This comment has been minimized.

@Kami

Kami May 16, 2016

Member

Please remove this, libcloud includes it's own custom User-Agent header (it includes version of Libcloud and driver user).

@Kami

Kami May 16, 2016

Member

Please remove this, libcloud includes it's own custom User-Agent header (it includes version of Libcloud and driver user).

Show outdated Hide outdated libcloud/dns/drivers/dnspod.py
def list_zones(self):
action = '/Domain.List'
data = {'user_token': self.key, 'format': 'json'}

This comment has been minimized.

@Kami

Kami May 16, 2016

Member

user_token and format attributes seem to be included in every request so you could maybe refactor this a bit and include those attributes somewhere else (e.g. inside the connection request method or similar) so the same piece of code doesn't need to be repeated all over.

@Kami

Kami May 16, 2016

Member

user_token and format attributes seem to be included in every request so you could maybe refactor this a bit and include those attributes somewhere else (e.g. inside the connection request method or similar) so the same piece of code doesn't need to be repeated all over.

This comment has been minimized.

@jetbird

jetbird May 18, 2016

Contributor

@Kami yes i thought about that, how to include that in every request?

@jetbird

jetbird May 18, 2016

Contributor

@Kami yes i thought about that, how to include that in every request?

This comment has been minimized.

@jetbird

jetbird May 20, 2016

Contributor

@Kami hey man any update on this? I have fixed the other problems, this is the only one left.

@jetbird

jetbird May 20, 2016

Contributor

@Kami hey man any update on this? I have fixed the other problems, this is the only one left.

This comment has been minimized.

@Kami

Kami May 21, 2016

Member

As mentioned in my comment above, you could refactor that functionality in a custom request method on the connection class.

You can find a similar example here - https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/cloudflare.py#L106

@Kami

Kami May 21, 2016

Member

As mentioned in my comment above, you could refactor that functionality in a custom request method on the connection class.

You can find a similar example here - https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/cloudflare.py#L106

Show outdated Hide outdated libcloud/dns/drivers/dnspod.py
e = sys.exc_info()[1]
if e.message == 'No domains':
return []
zones = self._to_zones(items=response.parse_body()['domains'])

This comment has been minimized.

@Kami

Kami May 16, 2016

Member

Libcloud already parses the body for you and is available via response.object attribute.

No need to call parse_body yourself.

@Kami

Kami May 16, 2016

Member

Libcloud already parses the body for you and is available via response.object attribute.

No need to call parse_body yourself.

Show outdated Hide outdated libcloud/dns/drivers/dnspod.py
data=data)
except DNSPodException:
e = sys.exc_info()[1]
if e.message in ['Domain is exists',

This comment has been minimized.

@Kami

Kami May 16, 2016

Member

You could save those messages in a constant, e.g. ZONE_ALREADY_EXISTS_ERROR_MSGS = [ ... ].

@Kami

Kami May 16, 2016

Member

You could save those messages in a constant, e.g. ZONE_ALREADY_EXISTS_ERROR_MSGS = [ ... ].

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 16, 2016

Member

Thanks 👍

I've added some in-line comments.

Member

Kami commented May 16, 2016

Thanks 👍

I've added some in-line comments.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 21, 2016

Member

@jetbird Did you push your latest changes?

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

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.).

@jetbird

This comment has been minimized.

Show comment
Hide comment
@jetbird

jetbird May 23, 2016

Contributor

@Kami done :)

Contributor

jetbird commented May 23, 2016

@Kami done :)

places error messages in lists, fixes the duplicate problem data param
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

updates the secrets file
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

asfgit pushed a commit that referenced this pull request May 25, 2016

updates the driver
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

asfgit pushed a commit that referenced this pull request May 25, 2016

updates code to pep8 standards, uses urlencode to support different p…
…ython versions

Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

asfgit pushed a commit that referenced this pull request May 25, 2016

removes unused import statements such as import urllib
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

asfgit pushed a commit that referenced this pull request May 25, 2016

fixes the lint for test_dnspod module
Closes #787

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

asfgit pushed a commit that referenced this pull request May 25, 2016

places error messages in lists, fixes the duplicate problem data para…
…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

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 25, 2016

Member

@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.

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

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 25, 2016

Member

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

Member

Kami commented May 25, 2016

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

@jetbird

This comment has been minimized.

Show comment
Hide comment
@jetbird

jetbird May 25, 2016

Contributor

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

Contributor

jetbird commented May 25, 2016

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

@jetbird

This comment has been minimized.

Show comment
Hide comment
@jetbird

jetbird May 25, 2016

Contributor

@Kami Tested. It works fine for me :)

Contributor

jetbird commented May 25, 2016

@Kami Tested. It works fine for me :)

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 25, 2016

Member

@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.).

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