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

[Libcloud-732] Add implementation for World Wide DNS provider #566

Merged
merged 1 commit into from Sep 25, 2015

Conversation

@aleGpereira
Copy link
Contributor

@aleGpereira aleGpereira commented Aug 26, 2015

@aleGpereira aleGpereira force-pushed the aleGpereira:LIBCLOUD-732_worldwidedns branch 2 times, most recently from f50e263 to 9a282c1 Aug 26, 2015
@aleGpereira
Copy link
Contributor Author

@aleGpereira aleGpereira commented Aug 26, 2015

I'm not sure if i followed the contributor guideline in the correct way. I'll expect for comments.

@Kami
Copy link
Member

@Kami Kami commented Aug 27, 2015

Great, thanks. I will look as soon as I can.

:rtype: ``str``
"""
if self._code_response(self.body):
codes = self.body.split('\r\n')

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

Using a regular expression and re.split would be safer (e.g. \r?\n) unless the body always contains carriage return (\r).

This comment has been minimized.

@aleGpereira

aleGpereira Aug 31, 2015
Author Contributor

Always contains the carriage. At least the documentation says that.

else:
return self._to_zones(zones.body)

def list_records(self, zone):

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

If you implement iterate_records you don't need to implement list_records since it's already implemented in the base driver as list(self._iterate_records(zone)).

:rtype: :class:`Record`
"""
zone = self.get_zone(zone_id)
records = self.list_records(zone)

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

I assume there isn't any more efficient way to retrieve a single record (e.g. filter by id or similar)?

:param domain: Zone domain name (e.g. example.com)
:type domain: ``str``
:param raw: Mode we use to do the update using zone file or not.

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

This is an extension (driver specific) argument so please prefix it with ex_ (ex_raw=False) and put it at the end after extra.

zone = self.get_zone(zone.id)
return zone

def update_record(self, record, name, type, data, entry):

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

Same here, please prefix entry with ex_ and put it after extra.

def update_record(self, record, name, type, data, extra=None, ex_position=1):
...
or
https://www.worldwidedns.net/dns_api_protocol_new_domain_reseller.asp
"""
if type == "master":

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

Minor style thing - please use single quotes around strings (minus docstrings).

from libcloud.dns.base import DNSDriver, Zone, Record


MAX_RECORD_ENTRIES = 40

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

It would be good to add a comment which explains what this constant represents.

self.update_zone(zone, zone.domain, extra=extra)
return True

def view_zone(self, domain, name_server):

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

This is an extension method so please prefix the method name with ex_ - def ex_view_zone(self, domain, name_server):

response = self.connection.request(action, params=params)
return response.object

def transfer_domain(self, domain, user_id):

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

Same here, method name needs ex_ prefix.

for code in codes:
if code in OK_CODES:
continue
elif code in ERROR_CODES:

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

An alternative which allows this method to be shorted would be to create a module level dict constant which maps code to the exception class (e.g. ERROR_CODE_TO_EXCEPTION_CLS) and using it here.

zone = self.update_zone(zone, zone.domain, ttl=ttl)
return zone

def create_record(self, name, zone, type, data, entry):

This comment has been minimized.

@Kami

Kami Aug 30, 2015
Member

Same here - entry needs to be prefixed with ex_.

@Kami
Copy link
Member

@Kami Kami commented Aug 30, 2015

@aleGpereira Thanks. I've added some comments.

Overall it looks good 👍, there are just some small things which need to be addressed.

aleGpereira pushed a commit to aleGpereira/libcloud that referenced this pull request Aug 31, 2015
@aleGpereira
Copy link
Contributor Author

@aleGpereira aleGpereira commented Aug 31, 2015

Ok, changes are pushed :).

aleGpereira pushed a commit to aleGpereira/libcloud that referenced this pull request Sep 1, 2015
@aleGpereira aleGpereira force-pushed the aleGpereira:LIBCLOUD-732_worldwidedns branch from eeaa936 to 45be41a Sep 1, 2015
@Kami
Copy link
Member

@Kami Kami commented Sep 4, 2015

Thanks. I just wanted to merge it, but I encountered an issue with a bunch of test failing:

======================================================================
ERROR: test_delete_zone_success (__main__.WorldWideDNSTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "libcloud/test/dns/test_worldwidedns.py", line 220, in test_delete_zone_success
    zone = self.driver.list_zones()[0]
  File "/w/lc/libcloud/libcloud/dns/drivers/worldwidedns.py", line 106, in list_zones
    return self._to_zones(zones.body)
  File "/w/lc/libcloud/libcloud/dns/drivers/worldwidedns.py", line 450, in _to_zones
    zone = self._to_zone(line)
  File "/w/lc/libcloud/libcloud/dns/drivers/worldwidedns.py", line 466, in _to_zone
    extra = {'HOSTMASTER': soa_block[0], 'REFRESH': soa_block[1],
IndexError: list index out of range

I had a look and it looks like something weird is going on with the splitting. As mentioned above, to be one the safe side and make it more robust I think we should also use regexed when splitting so we handle both, windows and linux new lines (with and without the carriage return).

On top of that, lint is also failing but it's a minor issue - https://travis-ci.org/apache/libcloud/builds/78253070

@aleGpereira Can you please have a look and fix it? Thanks.

@aleGpereira
Copy link
Contributor Author

@aleGpereira aleGpereira commented Sep 4, 2015

Ok. I'll check for that. Thanks.

@aleGpereira aleGpereira force-pushed the aleGpereira:LIBCLOUD-732_worldwidedns branch from f99dee7 to 0a7a629 Sep 7, 2015
@aleGpereira aleGpereira force-pushed the aleGpereira:LIBCLOUD-732_worldwidedns branch from 3f6d888 to 4eed2b2 Sep 7, 2015
@aleGpereira
Copy link
Contributor Author

@aleGpereira aleGpereira commented Sep 7, 2015

Ok. I've fix the problem. Sorry for that. I didn't ran with tox before. The regex is taking place now.

@Kami
Copy link
Member

@Kami Kami commented Sep 25, 2015

@aleGpereira One thing I forgot - since those contributions are pretty big, can you please also sign an ICLA - https://www.apache.org/licenses/#clas

@asfgit asfgit merged commit 4eed2b2 into apache:trunk Sep 25, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
asfgit pushed a commit that referenced this pull request Sep 25, 2015
Closes #566

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

Conflicts:
	CHANGES.rst
@Kami
Copy link
Member

@Kami Kami commented Sep 25, 2015

Merged, thanks!

@aleGpereira
Copy link
Contributor Author

@aleGpereira aleGpereira commented Oct 7, 2015

@Kami ICLA is signed and filed :).

@aleGpereira aleGpereira deleted the aleGpereira:LIBCLOUD-732_worldwidedns branch Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants