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: clarify setServer method #21469

Closed
wants to merge 4 commits into from
Closed

Conversation

Shivang44
Copy link
Contributor

@Shivang44 Shivang44 commented Jun 22, 2018

Added a note that that clarifies the fact that setServer does not
check subsequent servers when the first one produces a NOTFOUND
error.

  • [ x] documentation is changed or added
  • [x ] commit message follows commit guidelines

Refs: #21391

@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 Jun 22, 2018
@Shivang44 Shivang44 changed the title doc: clarify setServer method dns: clarify setServer method Jun 22, 2018
Added a note that that clarifies the fact that setServer does not
check subsequent servers when the first one produces a NOTFOUND
error.
@Shivang44 Shivang44 changed the title dns: clarify setServer method doc: clarify setServer method Jun 22, 2018
@lpinca
Copy link
Member

lpinca commented Jun 22, 2018

@Shivang44 hi,
as you can see from https://travis-ci.com/nodejs/node/jobs/130901081#L478 the linter is not happy.
Can you please fix that?

Thank you.

doc/api/dns.md Outdated
@@ -1010,6 +1010,8 @@ An error will be thrown if an invalid address is provided.
The `dnsPromises.setServers()` method must not be called while a DNS query is in
progress.

Note that this method works much like [resolve.conf in Linux](http://man7.org/linux/man-pages/man5/resolv.conf.5.html). That is, if attempting to resolve with the first server provided results in a `NOTFOUND` error, the `resolve` method will *not* attempt to resolve with subsequent servers provided. Fallback DNS servers will only be used if the earlier ones time out or result in some other error.
Copy link
Member

Choose a reason for hiding this comment

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

nit: line wrap at <= 80 chars

@cjihrig
Copy link
Contributor

cjihrig commented Jun 22, 2018

This only addresses dnsPromises.setServers(servers). You probably want to add similar text to dns.setServers(servers).

Lint was failing because doc was not under 80 lines.
doc/api/dns.md Outdated
Note that this method works much like
[resolve.conf](http://man7.org/linux/man-pages/man5/resolv.conf.5.html).
That is, if attempting to resolve with the first server provided results in a
`NOTFOUND` error, the `resolve` method will *not* attempt to resolve with
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: `resolve` -> `resolve()`?
(see STYLE_GUIDE.md about references to methods format)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, did not see that.

Added note about setServers in dnsPromises to dns as well.
Updated methods in doc to include parentheses.
@Shivang44
Copy link
Contributor Author

Thanks for all the feedback everyone. I updated to reflect all current comments.

@vsemozhetbyt
Copy link
Contributor

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Doc format LGTM.

vsemozhetbyt pushed a commit that referenced this pull request Jun 25, 2018
Added a note that that clarifies the fact that setServers() does not
check subsequent servers when the first one produces a NOTFOUND error.

PR-URL: #21469
Refs: #21391
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@vsemozhetbyt
Copy link
Contributor

Landed in 95205a6
Thank you!

targos pushed a commit that referenced this pull request Jun 26, 2018
Added a note that that clarifies the fact that setServers() does not
check subsequent servers when the first one produces a NOTFOUND error.

PR-URL: #21469
Refs: #21391
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants