FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: clarify what dns.setResolvers() affects by sam-github · Pull Request #25570 · nodejs/node · GitHub

/ node Public

doc: clarify what dns.setResolvers() affects - #25570

Closed
sam-github wants to merge 1 commit into
nodejs:masterfrom
sam-github:doc-clarify-setservers
ConversationCommits (1)ChecksFiles changed
Closed

doc: clarify what dns.setResolvers() affects#25570
sam-github wants to merge 1 commit into
nodejs:masterfrom
sam-github:doc-clarify-setservers

Conversation

sam-github commented Jan 18, 2019

Copy link
Copy Markdown
Contributor

It does not affect dns.lookup().

cf. #25560

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot commented Jan 18, 2019

Copy link
Copy Markdown
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2288/pipeline

nodejs-github-bot added dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations. labels Jan 18, 2019
sam-github force-pushed the doc-clarify-setservers branch from 5a91f8d to ccec12a Compare January 18, 2019 18:12
vsemozhetbyt approved these changes Jan 18, 2019
View reviewed changes
Comment thread doc/api/dns.md Outdated

vsemozhetbyt Jan 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: unneeded empty line?

szmarczak reviewed Jan 18, 2019
View reviewed changes
Comment thread doc/api/dns.md Outdated
addaleax reviewed Jan 18, 2019
View reviewed changes
Comment thread doc/api/dns.md Outdated
sam-github force-pushed the doc-clarify-setservers branch from ccec12a to 905a392 Compare January 23, 2019 22:44

sam-github commented Jan 23, 2019

Copy link
Copy Markdown
Contributor Author

Also, calling dns.setServers() should always be okay in recent versions of Node. Calling resolver.setServers() is also okay, but will yield an error if there are active queries.

I haven't changed this text, and don't want to get into it here. I'll add it to my todo list. Can you reference the PR that did this? It'll need YAML change markers. If you don't know, I'll track it down some time.

The structure of the docs are confusing. The Resolver class refers to the global docs, but it really be the other way around, the Resolver methods should be documented, and the globals should state that there is a global Resolver. An astute reader could guess that its implemented like this, but it isn't clearly stated.

addaleax commented Jan 23, 2019

Copy link
Copy Markdown
Member

I think the relevant PRs were #14891 (error out when active queries are on the resolver + change global default resolver when calling dns.setServers()) and, if you want, #14518 (adding the Resolver class).

doc: clarify what dns.setResolvers() affects
d003bcd 
It does not affect dns.lookup().
sam-github force-pushed the doc-clarify-setservers branch from 905a392 to d003bcd Compare January 24, 2019 22:10

sam-github commented Jan 24, 2019

Copy link
Copy Markdown
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2379/

sam-github commented Jan 25, 2019

Copy link
Copy Markdown
Contributor Author

Landed in 0ce615c

sam-github closed this Jan 25, 2019
sam-github deleted the doc-clarify-setservers branch January 25, 2019 18:18
addaleax pushed a commit that referenced this pull request Jan 28, 2019
doc: clarify what dns.setResolvers() affects
48149cf 
It does not affect dns.lookup().

PR-URL: #25570
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos mentioned this pull request Jan 29, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
doc: clarify what dns.setResolvers() affects
6d49ad1 
It does not affect dns.lookup().

PR-URL: #25570
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
doc: clarify what dns.setResolvers() affects
20355c3 
It does not affect dns.lookup().

PR-URL: #25570
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
doc: clarify what dns.setResolvers() affects
bb96d79 
It does not affect dns.lookup().

PR-URL: #25570
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
lpinca lpinca approved these changes
+2 more reviewers
szmarczak szmarczak approved these changes
vsemozhetbyt vsemozhetbyt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL