Dinahosting provider #414
Dinahosting provider #414
Conversation
|
Hi. I'm an automated pull request bot named CapsuleCD. I handle testing, versioning and package releases for this project.
If you're interested in learning more about CapsuleCD, or adding it to your project, you can check it out here |
lexicon/providers/dinahosting.py
Outdated
| payload = self._post('', data) | ||
|
|
||
| #content_obj = self._parse_record_content(rtype, content) | ||
| #content = content_obj['data'] |
There was a problem hiding this comment.
Some commented code to remove.
There was a problem hiding this comment.
Agreed. While I was at it I modified the _list_records to correct the lint issue and followed one of your suggestions from another PR about extracting the pseudo-id to its own _identifier method.
lexicon/providers/dinahosting.py
Outdated
|
|
||
| def _update_record(self, identifier, rtype=None, name=None, content=None): | ||
| if identifier: | ||
| records = self._list_records(rtype) |
There was a problem hiding this comment.
If this is not too heavy in term of API call, I think you could just get all records in the zone, not filtering on the rtype, then find the matching identifier. It allows a slightly wider usage of update_record.
| if not matching_records: | ||
| raise Exception("No matching records found. Cannot update.") | ||
| if len(matching_records) > 1: | ||
| raise Exception("Multiple matching records found. Cannot update.") |
There was a problem hiding this comment.
It is your choice here, since we did not define precisely the spec when multiple records are encountered during an update. My current choice, on recent providers, is to take the first one, and log.warn about the fact that multiple records have been found.
Your choice here :)
lexicon/providers/dinahosting.py
Outdated
|
|
||
| def _delete_record(self, identifier=None, rtype=None, name=None, content=None): | ||
| if identifier: | ||
| records = self._list_records(rtype) |
There was a problem hiding this comment.
Same as above in update method, to just get all records in the zone then filter.
|
NB: Don't mind the failure in AppVeyor if it is about Python 3.4 environment, it is broken for now. |
|
Hey @iperurena Thanks again for the PR! 🎉 |
|
Hey @AnalogJ Regarding the unsupported type list. To my knowledge, this API or the platform itself does not offer any mechanism to define LOC or SOA type records. As for the other types, I found out that the API either supports them only partially or the methods are somewhat designed to work as a backend to the platform's UI rather than being used programmatically. Thus, I deemed them unsuitable for the purposes of this library. To elaborate, MX records only work when using what the API calls "simplified MX". It won't let you define each record's priority and it introduces a weird dependency with A type records when listing/adding/deleting the records. |
|
Looking forward to your feedback @AnalogJ , @adferrand |
There was a problem hiding this comment.
For me, all observations have been answered, and the limitations on the API justify to limit the record types that will be handled by Lexicon. For the typical purpose of Lexicon, the coverage is sufficient.
Is it OK for you also @AnalogJ?
|
OK. I think we can merge! |
|
Just saw the message @adferrand, thank you for all your feedback! |
* Add dinahosting provider stub * Added provider logic & test recordings. Integration tests passed. * Added myself to codeowners file * Fixed the lint. Removed rtype filter in _update & _delete methods. Added _identifier helper * Update integration test recordings. Add provider to readme
Add a provider for Dinahosting DNS API.