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-772 driver class for GoDaddy DNS #640

Conversation

@tonybaloney
Copy link
Contributor

tonybaloney commented Nov 24, 2015

  • Implement driver methods
  • Document usage and examples
  • Add tests and responses based on developer.godaddy.com
tonybaloney added 2 commits Nov 24, 2015
Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
Fix lint
Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
@Kami
Copy link
Member

Kami commented Nov 25, 2015

As far as test fixtures go - I would recommend to use actual responses you get back when talking to the API.

Maybe times before we encountered an issue with out of date or incorrect docs (I just encountered that issue with CloudFlare DNS driver a couple of days ago - some of the responses in the docs were incorrect and out of date). Sadly it looks like most of the provider documentation is still written manually and not automatically generated from code.

tonybaloney added 5 commits Nov 27, 2015
…s, add and update records, cancel domains, check availability, validate purchase requests

Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
@tonybaloney tonybaloney changed the title [WIP] LIBCLOUD-772 driver class for GoDaddy DNS LIBCLOUD-772 driver class for GoDaddy DNS Nov 27, 2015
@tonybaloney
Copy link
Contributor Author

tonybaloney commented Nov 27, 2015

GoDaddy records don't have IDs, and when you update or query them you combine the name and type, so I turned that into the ID. Just for get_record, for lack of an alternative.

@tonybaloney
Copy link
Contributor Author

tonybaloney commented Nov 27, 2015

@Kami your thoughts please. this is complete now

@Kami
Copy link
Member

Kami commented Nov 27, 2015

GoDaddy records don't have IDs, and when you update or query them you combine the name and type, so I turned that into the ID. Just for get_record, for lack of an alternative.

Yeah, I believe GCE driver does something like that as well.

@tonybaloney
Copy link
Contributor Author

tonybaloney commented Nov 27, 2015

@Kami as per your suggestion i used real API responses for the tests as well. except the "purchase domain" one, that would get expensive

if not self.body:
return None
# json.loads doesn't like the regex expressions used in godaddy schema
self.body = self.body.replace('\\.', '\\\\.')

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Hm, ok :P

This comment has been minimized.

@tonybaloney

tonybaloney Nov 27, 2015 Author Contributor

https://www.google.com.au/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=json%20loads%20invalid%20escape yeah I'm not the only one. the other option is to introduce cjson, but adding another prereq to the project for just one regex statement seems excessive

host=host, port=port,
shopper_id=str(shopper_id))

def _to_zones(self, items):

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Minor code style thing - please put "'private" methods to the bottom - https://libcloud.readthedocs.org/en/latest/development.html#function-and-method-ordering

ttl = item['ttl']
type = self._string_to_record_type(item['type'])
name = item['name']
id = '{0}:{1}'.format(name, type)

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Please use '%s' for string interpolation. We still support Python 2.5 and 2.6 (we will probably drop support for 2.5 in the near future) where .format() is not available.

record = Record(id=id, name=name,
type=type, data=item['data'],
zone=zone, driver=self,
extra={'ttl': ttl})

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

I recently merged PR which adds ttl attribute to the Record class so you can use this now - #639

:return: ``list`` of :class:`Record`
"""
result = self.connection.request(
'/v1/domains/%s/records' % zone.domain).object

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Minor code style / safety thing - always when using string interpolation wrap arguments in parenthesis - it prevents accidental errors which could arise if user adds another argument, but forgets to add parenthesis (we had those issues in production before, ideally tests should discover that, but still).

'/v1/domains/{0}/records'.format(
zone.domain), method='PATCH',
data=[new_record])
id = '{0}:{1}'.format(name, type)

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Same here - please use %.

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Maybe utility method would be handy - _get_id_for_record or similar.

if extra is None:
extra = {}
new_record = {}
if type is RecordType.SRV:

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

It seems there is a lot of duplication code between this method and create record - would be good to refactor common stuff in a utility method.

Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
'name': name,
'data': data,
'priority': 1,
'ttl': extra['ttl'] if hasattr(extra, 'ttl') else 5,

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

I just noticed you use hasattr. This won't work since hasattr only works on class instances. For dictionaries you should either use key in dict or dict.get(key, default_value).

zone.domain), method='PATCH',
data=[new_record])
id = '{0}:{1}'.format(name, type)
return Record(

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Same here, you can now also pass ttl to the Record class.

zone_id,
parts[1],
parts[0])).object
if len(result) is 0:

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

This would probably work most of the time, but it's kinda Python implementation specific so you should only use is to compare for True, False and None since is checks the reference and not the actual value.

See https://stackoverflow.com/questions/1504717/why-does-comparing-strings-in-python-using-either-or-is-sometimes-produce

parts[1],
parts[0])).object
if len(result) is 0:
raise GoDaddyDNSException("Could not locate record")

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Should throw RecordDoesNotExistError

@Kami
Copy link
Member

Kami commented Nov 27, 2015

Thanks.

Added some comments.

@Kami
Copy link
Member

Kami commented Nov 27, 2015

Ah, you are fast! :)

tonybaloney added 3 commits Nov 27, 2015
* added method for _format_record to avoid DRY in create and update record
* raise record does not exist error

Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
…ords

Signed-off-by: Anthony Shaw <anthony.p.shaw@gmail.com>
@tonybaloney
Copy link
Contributor Author

tonybaloney commented Nov 27, 2015

  • fix string interpolation for python 2,5
  • merge ttl update and use new record type
  • wrap string formats in parenthesis
  • create method to format a new record request for create and update record
  • dont use hasattr
  • use proper equality comparer
  • create utility method for format id of record
  • throw record does not exist error
@tonybaloney
Copy link
Contributor Author

tonybaloney commented Nov 27, 2015

ok @Kami done. very thorough code review!

if extra is None:
extra = {}
new_record = {}
if type is RecordType.SRV:

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

Same here, should also use ==.

parts[1],
parts[0])).object
if len(result) == 0:
raise RecordDoesNotExistError()

This comment has been minimized.

@Kami

Kami Nov 27, 2015 Member

I don't think this will work - you need to pass the following arguments to the constructor - value, driver, record_id

@Kami
Copy link
Member

Kami commented Nov 27, 2015

A couple of more things.

Besides that, LGTM, +1

@asfgit asfgit closed this in d4fba98 Nov 27, 2015
@Kami
Copy link
Member

Kami commented Feb 23, 2016

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

2 participants
You can’t perform that action at this time.