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

Register priority for MX records #718

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
2 participants
@zepheiryan
Contributor

zepheiryan commented Feb 29, 2016

As is, this fails to split out the priority value from the raw record value, resulting in a key error for missing priority when trying to export in BIND format. This is the quick hack I did to get the function to return a more useful Record.

Register priority for MX records
As is, this fails to split out the priority value from the raw record value, resulting in a key error for missing priority when trying to export in BIND format.  This is the quick hack I did to get the function to return a more useful Record.
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 29, 2016

Member

Thanks for the contribution.

Member

Kami commented Feb 29, 2016

Thanks for the contribution.

value = record['value']
if record['type'] == 'MX':
extras['priority'] = record['value'].split(' ')[0]
value = record['value'].split(' ')[1]

This comment has been minimized.

@Kami

Kami Feb 29, 2016

Member

Just curious why you split the value here and take the second (and not the first item) - it would help if I could see how the value looks like :)

@Kami

Kami Feb 29, 2016

Member

Just curious why you split the value here and take the second (and not the first item) - it would help if I could see how the value looks like :)

This comment has been minimized.

@zepheiryan

zepheiryan Feb 29, 2016

Contributor

record['value'] looks like 15 aspmx.l.google.com - I didn't try it, but I expect leaving the value as that raw value would spit out ...15 15 aspmx... when converted to BIND speak.

Given that I don't have an exhaustive knowledge of DNS record types off the top of my head (or how Gandi returns them all), I expect this is an incomplete patch that happened to work for the (or my) Gandi MX case. I'm not sure how your team prefers to handle that, but I did want it to at least start on the way in so folks (or just me) don't have to monkey patch locally.

@zepheiryan

zepheiryan Feb 29, 2016

Contributor

record['value'] looks like 15 aspmx.l.google.com - I didn't try it, but I expect leaving the value as that raw value would spit out ...15 15 aspmx... when converted to BIND speak.

Given that I don't have an exhaustive knowledge of DNS record types off the top of my head (or how Gandi returns them all), I expect this is an incomplete patch that happened to work for the (or my) Gandi MX case. I'm not sure how your team prefers to handle that, but I did want it to at least start on the way in so folks (or just me) don't have to monkey patch locally.

This comment has been minimized.

@Kami

Kami Mar 1, 2016

Member

That's for clarifying it.

This does indeed seem like a bug / inconsistency in the Gandi driver. value attribute should indeed only contain the value and the priority should be stored in extra.

@Kami

Kami Mar 1, 2016

Member

That's for clarifying it.

This does indeed seem like a bug / inconsistency in the Gandi driver. value attribute should indeed only contain the value and the priority should be stored in extra.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 29, 2016

Member

Test case for list_records method which returns a MX record would also be nice :)

Member

Kami commented Feb 29, 2016

Test case for list_records method which returns a MX record would also be nice :)

@asfgit asfgit closed this in f29960f Mar 1, 2016

asfgit pushed a commit that referenced this pull request Mar 1, 2016

asfgit pushed a commit that referenced this pull request Mar 1, 2016

asfgit pushed a commit that referenced this pull request Mar 1, 2016

asfgit pushed a commit that referenced this pull request Mar 1, 2016

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 1, 2016

Member

Pushed some fixes and improvements (1d0b2ee, 2929692), added a test case (bd3c1f0) and merged changes into trunk.

Thanks!

Member

Kami commented Mar 1, 2016

Pushed some fixes and improvements (1d0b2ee, 2929692), added a test case (bd3c1f0) and merged changes into trunk.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment