Skip to content

dns, errors: Migrate to use internal/errors #14212

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

dns, errors: Migrate to use internal/errors #14212

wants to merge 1 commit into from

Conversation

@starkwang
Copy link
Contributor

@starkwang starkwang commented Jul 13, 2017 •
edited
Loading

Migrate dns errors to use internal/errors.

Ref: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

dns, errors

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 13, 2017
@starkwang starkwang force-pushed the dns-internal-errors branch from ebfd843 to be61d5d Compare July 13, 2017 06:08
jasnell
jasnell reviewed Jul 14, 2017
View reviewed changes
doc/api/errors.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps ERR_DNS_SET_SERVERS_FAILED?

jasnell
jasnell requested changes Jul 14, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Very close! thank for doing this. Left a comment I'd like to see addressed.

@starkwang starkwang force-pushed the dns-internal-errors branch 2 times, most recently from 1ae02d3 to 56d8131 Compare July 18, 2017 07:59
jasnell
jasnell approved these changes Jul 19, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 19, 2017
XadillaX
XadillaX approved these changes Jul 21, 2017
View reviewed changes
Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

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

LGTM

@XadillaX
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/9296/

@starkwang
Copy link
Contributor Author

starkwang commented Jul 22, 2017 •
edited
Loading

One failure in CI looks unrelated.
https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=0,label=pi1-raspbian-wheezy/9392/console

  ...
not ok 23 parallel/test-child-process-fork-exec-path
  ---
  duration_ms: 8.891
  severity: fail
  stack: |-
  ...

refack
refack approved these changes Jul 24, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Some nits

lib/dns.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

new indentation rule requires aligning to the call (
#14403

lib/dns.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

i

lib/internal/errors.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

probably better (err, servers) => `c-ares failed to set servers: "${err}" [${servers}]`

test/parallel/test-c-ares.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

New feature of expectsError can be used without assert.throws:

common.expectsError(
  dns.resolve('www.google.com', val),
  {
    code: 'ERR_INVALID_OPT_VALUE',
    type: TypeError,
    message: `The value "${val}" is invalid for option "rrtype"`
  }
);

refack
refack reviewed Jul 24, 2017
View reviewed changes
doc/api/errors.md Outdated
Copy link
Contributor

@refack refack Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Used when `c-ares` failed to set the DNS servers.

@jasnell jasnell mentioned this pull request Jul 24, 2017
Closed
80 tasks
@refack
Copy link
Contributor

refack commented Jul 24, 2017

@nodejs/ctc @fhinkel @mcollina @mhdawson

@starkwang starkwang force-pushed the dns-internal-errors branch from 56d8131 to 0653452 Compare July 24, 2017 04:31
@starkwang
Copy link
Contributor Author

Pushed commit to address comments.

mcollina
mcollina approved these changes Jul 24, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor

refack commented Jul 24, 2017

@starkwang thank you.
CI: https://ci.nodejs.org/job/node-test-pull-request/9319/

@refack
Copy link
Contributor

refack commented Jul 24, 2017

Landed in 9cb390d

@refack refack closed this Jul 24, 2017
refack pushed a commit that referenced this pull request Jul 24, 2017
@starkwang @refack
errors: migrate dns to use internal/errors
9cb390d 
PR-URL: #14212
Refs: #11273
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@starkwang starkwang mentioned this pull request Jul 27, 2017
Closed
4 tasks
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

+2 more reviewers

@refack refack refack approved these changes

@XadillaX XadillaX XadillaX 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. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@starkwang @XadillaX @refack @mcollina @jasnell @nodejs-github-bot