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

Remove dnspython deprecation warning #5117

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tripleee
Copy link
Member

Where previously we used dns.resolver.query(), switch to d.r.resolve()

Complete compatibility would require d.r.resolve(search=True) but this works without it so far.

Perhaps see also rthalley/dnspython#581

Where previously we used dns.resolver.query(), switch to d.r.resolve()

Complete compatibility would require d.r.resolve(search=True) but this works without it so far.

Perhaps see also rthalley/dnspython#581
@tripleee tripleee changed the title Remove dnspython depreciation warning Remove dnspython deprecation warning Nov 12, 2020
@makyen
Copy link
Contributor

makyen commented Nov 12, 2020

This has dependencies and interrelated issues.

IIRC, the primary issue surrounding this was that the replacement for the deprecated function wasn't available in a dnspython version which is compatible with Python 3.5. So, fixing this issue requires:

  • We stop supporting 3.5 (which we can now). This was mostly waiting for Python 3.9.0 to be released, which happened about a month ago.
    • Our officially supported versions and test structure should migrate to testing 3.6, 3.7, 3.8, and 3.9. This should be done in a separate PR.
    • We are considering removing CI testing being done by Travis, due to Travis changing how they handle open source projects (basically Travis is changing to make it a pain in the rear to use them). We should go ahead and do this for the SmokeDetector repository. Migrating the userscripts repository is something I've planned. We should also consider migrating the Metasmoke repository away from Travis. We would then still have the ChatExchange repo, which would need to be looked at. However, everything other than SD has a much lower CI volume requirement and may be reasonable to support on Travis for a while.
      Again, this should be a separate PR.
  • We need to change the requirements.txt file to indicate a minimum version for dnspython which has the replacement function.
    • Updating requirements and switching to code that requires a new version means that such update must be coordinated with all current SmokeDetector runners, so we don't kill active or standby instances.

Copy link
Contributor

@makyen makyen left a comment

Choose a reason for hiding this comment

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

See previous comment.

@tripleee
Copy link
Member Author

Related: #4514 #4841

@tripleee tripleee added the status: pending Just a few more things before it's live! label Nov 13, 2020
teward added a commit that referenced this pull request Nov 16, 2020
Looks like dnsresolver class with the modifiable resolver class in here for configuration and use in the `!!/ip` call we used a while ago for DNS lookups of things (when we were debugging differing DNS records for domains) did not get the changes that #5117 was going to implement.  So, implement.
@teward
Copy link
Member

teward commented Nov 16, 2020

Complete compatibility would require d.r.resolve(search=True) but this works without it so far.

I'm curious, but why would we need search=True to make things work right? search uses local search domains as set by the system which for SmokeDetectors should be none.

From the DNSPython configuration documents, this is what 'search' does:

search, a bool or None, determines whether the search list configured in the system’s resolver configuration are used for relative names, and whether the resolver’s domain may be added to relative names.

We won't need search=True configured for our resolutions, because none of them are system-domain-local.

Therefore, that function being added or not is irrelevant, I just tested the difference between .resolve and .query for our code locally and for searches for websites it will not affect the searches.

tripleee pushed a commit to tripleee/SmokeDetector that referenced this pull request Nov 16, 2020
Looks like dnsresolver class with the modifiable resolver class in here for configuration and use in the `!!/ip` call we used a while ago for DNS lookups of things (when we were debugging differing DNS records for domains) did not get the changes that Charcoal-SE#5117 was going to implement.  So, implement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: pending Just a few more things before it's live!
Development

Successfully merging this pull request may close these issues.

3 participants