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

doc: note about dns ANY queries / RFC 8482 #26695

Closed
wants to merge 2 commits into from

Conversation

tlhunter
Copy link
Contributor

This was inspired by the Cloudflare post, RFC8482 - Saying goodbye to ANY / RFC 8482.

The post calls out that the ANY query isn't always honored by various services and so this PR warns the developer that such discrepancies can exist and that they may need to call other methods one at a time instead.

Checklist

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations. labels Mar 15, 2019
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

🎉

doc/api/dns.md Outdated
Note that the operator of an authoritative DNS server might choose not to
respond to `ANY` queries for reasons of local policy, motivated by security,
performance, or other reasons. For that reason you may want to instead call
individual methods like `resolve4()`, `resolveMx()`, etc. For more information
Copy link
Member

Choose a reason for hiding this comment

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

Can you make these into links to the other functions in this page?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Would the text/markdown below work for you instead? I'm trying to avoid unnecessarily-lengthy sentences and unneeded details. I'm also enforcing some style rules here (like avoid personal pronouns).

Note that the operator of an authoritative DNS server might choose not to
respond to `ANY` queries. It may be better to call individual methods like
`resolve4()`, `resolveMx()`, and so on. For more information, see [RFC 8482][].

I'd also be OK with upgrading "It may be better" to "It is almost always better" or "It is always better" or "It is better", or "Whenever possible, use..." I went with "It may be better" because it seemed to most closely reflect your original text ("you may want to"). But if a stronger statement is correct and appropriate, let's use it.

@Trott
Copy link
Member

Trott commented Mar 16, 2019

(By the way, the style guide is at https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md. Worth looking at if you plan on writing lots of docs here, but less so if you plan on dropping a little piece in here or there and don't mind someone like me nit-picking the crud out of it. 😆 )

@tlhunter
Copy link
Contributor Author

I modified the text to be more concise and use reference-style links.

Note that I'm using the RFC XXXX syntax, which I believe is the most correct way (e.g. check the title of an actual RFC document. However, throughout the docs, the formats RFC XXXX, RFCXXXX, and rfcXXXX are used. It might be worth normalizing these everywhere.

@Trott
Copy link
Member

Trott commented Mar 16, 2019

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2019
doc/api/dns.md Outdated Show resolved Hide resolved
Co-Authored-By: tlhunter <me@thomashunter.name>
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt pushed a commit that referenced this pull request Mar 17, 2019
PR-URL: #26695
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@vsemozhetbyt
Copy link
Contributor

Landed in 3b6344c (with a nit fix in bottom references sorting).
Thank you!

@richardlau
Copy link
Member

Note that I'm using the RFC XXXX syntax, which I believe is the most correct way (e.g. check the title of an actual RFC document. However, throughout the docs, the formats RFC XXXX, RFCXXXX, and rfcXXXX are used. It might be worth normalizing these everywhere.

We can normalize on RFC XXXX via the doc lint rules. I've raised nodejs/remark-preset-lint-node#15 as a first step.

@tlhunter tlhunter deleted the doc-dns branch March 18, 2019 18:57
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26695
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Mar 27, 2019
PR-URL: #26695
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
PR-URL: #26695
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants