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

Fix ipadiscovery #681

Closed
wants to merge 3 commits into from
Closed

Fix ipadiscovery #681

wants to merge 3 commits into from

Conversation

alex-zel
Copy link

@alex-zel alex-zel commented Apr 2, 2017

Sort SRV records for LDAP/KRB based on priority.

Alex Zeleznikov and others added 2 commits April 2, 2017 11:53
Sort SRV records for LDAP/KRB based on priority.
@tiran
Copy link
Member

tiran commented Apr 3, 2017

You can simplify your code a lot with the operator module and sorted(key) trick:

https://docs.python.org/3/library/operator.html#operator.attrgetter
https://docs.python.org/3/library/functions.html#sorted

import operator
answers = resolver.query(qname, rdatatype.SRV)
answers = sorted(answer, key=operator.attrgetter('priority'))

Please squash your changes into one commit.

@martbab
Copy link
Contributor

martbab commented Apr 3, 2017

Hi Alex, a few comments:

1.) please see PEP8 guide for correct Python formatting https://www.python.org/dev/peps/pep-0008/ namely, do not use tabs but 4 spaces for indentation.

2.) I do not see much value in sorting TXT records. We are searching for _kerberos TXT record which should occur only once in DNS domain.

3.) please use a more concise sorting mechanism mentioned by @tiran, your way is very unpythonic and inefficient (list insertions).

@@ -521,7 +530,16 @@ def ipadnssearchkrbrealm(self, domain=None):
root_logger.debug("Search DNS for TXT record of %s", qname)

try:
answers = resolver.query(qname, rdatatype.TXT)
answers = []
dns_answers = resolver.query(qname, rdatatype.SRV)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You changed TXT to SRV here. It should stay TXT RR and it has no priority attribute

@alex-zel alex-zel closed this Apr 4, 2017
@alex-zel alex-zel deleted the fix-ipadiscovery branch April 4, 2017 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants