Solve fix inconsistent (hostname vs host) in DNS. #20892 by shakeelmohamed · Pull Request #23572 · nodejs/node

shakeelmohamed · 2018-10-12T18:11:53Z

This PR replaces #21471 and closes #20892 by fixing a linting issue by a line being over 80 characters after the host -> hostname rename.

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

I found there's a linting issue with PR #21471:

$ make lint-js-fix

$NODE_SOURCE/node/test/parallel/test-dns.js
  247:1  error  Line 247 exceeds the maximum line length of 80  max-len

✖ 1 problem (1 error, 0 warnings)

make: *** [lint-js-fix] Error 1

The following patch addresses this issue:

diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js
index c6f5995cde..98d3670d47 100644
--- a/test/parallel/test-dns.js
+++ b/test/parallel/test-dns.js
@@ -244,7 +244,8 @@ dns.lookup('', {
   const err = {
     code: 'ERR_MISSING_ARGS',
     type: TypeError,
-    message: 'The "hostname", "port", and "callback" arguments must be specified'
+    message: 'The "hostname", "port", and "callback" arguments must be ' +
+    'specified'
   };
 
   common.expectsError(() => dns.lookupService('0.0.0.0'), err);

I've verified this locally:

$ make lint-ci    
Running JS linter...
Running C++ linter...
Total errors found: 0
Running Markdown linter on misc docs...
Running Markdown linter on docs...
rm -f -r test/addons/??_*/
[ -x ./node ] && ./node tools/doc/addon-verify.js || node tools/doc/addon-verify.js
Wrote $NODE_SOURCE/node/test/addons/01_function_arguments/test.js
Wrote $NODE_SOURCE/node/test/addons/02_callbacks/addon.cc
Wrote $NODE_SOURCE/node/test/addons/01_function_arguments/addon.cc
Wrote $NODE_SOURCE/node/test/addons/01_function_arguments/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/02_callbacks/test.js
Wrote $NODE_SOURCE/node/test/addons/02_callbacks/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/03_object_factory/addon.cc
Wrote $NODE_SOURCE/node/test/addons/03_object_factory/test.js
Wrote $NODE_SOURCE/node/test/addons/03_object_factory/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/04_function_factory/test.js
Wrote $NODE_SOURCE/node/test/addons/05_wrapping_c_objects/myobject.h
Wrote $NODE_SOURCE/node/test/addons/04_function_factory/addon.cc
Wrote $NODE_SOURCE/node/test/addons/04_function_factory/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/05_wrapping_c_objects/addon.cc
Wrote $NODE_SOURCE/node/test/addons/06_factory_of_wrapped_objects/addon.cc
Wrote $NODE_SOURCE/node/test/addons/05_wrapping_c_objects/myobject.cc
Wrote $NODE_SOURCE/node/test/addons/05_wrapping_c_objects/test.js
Wrote $NODE_SOURCE/node/test/addons/05_wrapping_c_objects/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/06_factory_of_wrapped_objects/test.js
Wrote $NODE_SOURCE/node/test/addons/06_factory_of_wrapped_objects/myobject.h
Wrote $NODE_SOURCE/node/test/addons/07_passing_wrapped_objects_around/addon.cc
Wrote $NODE_SOURCE/node/test/addons/07_passing_wrapped_objects_around/myobject.cc
Wrote $NODE_SOURCE/node/test/addons/06_factory_of_wrapped_objects/myobject.cc
Wrote $NODE_SOURCE/node/test/addons/07_passing_wrapped_objects_around/myobject.h
Wrote $NODE_SOURCE/node/test/addons/07_passing_wrapped_objects_around/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/06_factory_of_wrapped_objects/binding.gyp
Wrote $NODE_SOURCE/node/test/addons/08_void_atexitcallback_args/test.js
Wrote $NODE_SOURCE/node/test/addons/08_void_atexitcallback_args/addon.cc
Wrote $NODE_SOURCE/node/test/addons/07_passing_wrapped_objects_around/test.js
Wrote $NODE_SOURCE/node/test/addons/08_void_atexitcallback_args/binding.gyp
touch test/addons/.docbuildstamp
Running C++ linter on addon docs...
Total errors found: 0

shakeelmohamed · 2018-10-14T05:01:33Z

@addaleax hey, any guidance on the CI failure?
The command git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx core-validate-commit --no-validate-metadata shows an issue with commit 9d7895c567e8f38abfff35da1b6d6d6a0a06f9aa which isn't part of this PR.
Am I missing something obvious?

lundibundi · 2018-10-14T14:02:47Z

lib/dns.js

  var req = new GetNameInfoReqWrap();
  req.callback = callback;
-  req.host = host;
+  req.hostname = hostname;


Just to understand this: isn't this name used later on during the resolution on the libuv side? And if it's not then what's the use of assigning it here?
/cc @addaleax @jasnell

@lundibundi I think the only use we currently have for it is this.hostname in onlookupservice() above? I might be missing something, though.

Looks like it as I haven't found anything down to the libuv/unix/getnameinfo, though I may have missed it. Though it looks kind of strange that we pass hostname both via req/this and as an argument in case of success.

Well, anyway, CI is almost green so this is fine =).

Adds tests for a few error conditions. Also, adds tests to make sure the dynamically generated url is correct. PR-URL: nodejs#23573 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> PR-URL: nodejs#23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax · 2018-10-16T11:38:05Z

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

Adds tests for a few error conditions. Also, adds tests to make sure the dynamically generated url is correct. PR-URL: #23573 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Trott · 2018-10-18T02:08:10Z

@addaleax hey, any guidance on the CI failure?
The command git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx core-validate-commit --no-validate-metadata shows an issue with commit 9d7895c which isn't part of this PR.
Am I missing something obvious?

That's a bug in either Travis or (more likely) our .travis.yml implementation. You can ignore it. I'll see if I can figure it out, but for the moment, I'm kinda stumped.

Trott · 2018-10-18T02:08:52Z

(I'll try to fix it by having Travis skip the commit message linting if the environment variable is not populated.)

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: nodejs#23572 (comment) Refs: nodejs#22842 (comment)

Trott · 2018-10-18T04:53:12Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/17967/

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>

addaleax · 2018-10-21T04:05:51Z

Resume build again: https://ci.nodejs.org/job/node-test-pull-request/18018/

Fixes: nodejs#20892 PR-URL: nodejs#23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Trott · 2018-10-21T04:58:08Z

Landed in beb0f03.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Adds tests for a few error conditions. Also, adds tests to make sure the dynamically generated url is correct. PR-URL: #23573 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

UlisesGascon and others added 4 commits June 22, 2018 17:41

Solve fix inconsistent (hostname vs host) in DNS. nodejs#20892

fab9b15

Merge branch 'master' of https://github.com/nodejs/node

cb0ceee

Update test validation for nodejs#21471

b9e2916

lint: fix line length for test/parallel/test-dns.js

0bd9cf4

nodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Oct 12, 2018

shakeelmohamed changed the title ~~20892~~ Solve fix inconsistent (hostname vs host) in DNS. #20892 Oct 12, 2018

targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018

Fix additional inconsistencies (hostname vs host) in DNS. nodejs#20892

8c1e1d0

jasnell approved these changes Oct 12, 2018

View reviewed changes

addaleax approved these changes Oct 12, 2018

View reviewed changes

lundibundi reviewed Oct 14, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2018

Trott added a commit to Trott/io.js that referenced this pull request Oct 18, 2018

tools: do not lint commit message if var undefined

83628a4

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: nodejs#23572 (comment) Refs: nodejs#22842 (comment)

Trott mentioned this pull request Oct 18, 2018

tools: do not lint commit message if var undefined #23725

Closed

2 tasks

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 21, 2018

dns: fix inconsistent (hostname vs host)

beb0f03

Fixes: nodejs#20892 PR-URL: nodejs#23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Trott closed this Oct 21, 2018

jasnell pushed a commit that referenced this pull request Oct 21, 2018

dns: fix inconsistent (hostname vs host)

dd75624

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax mentioned this pull request Nov 6, 2018

doc: fix some inconsistent use of hostname #24199

Closed

3 tasks

MylesBorins added the backport-requested-v8.x label Nov 26, 2018

MylesBorins pushed a commit that referenced this pull request Nov 26, 2018

dns: fix inconsistent (hostname vs host)

b2252ea

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins added the land-on-v10.x label Nov 26, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

rvagg pushed a commit that referenced this pull request Nov 28, 2018

dns: fix inconsistent (hostname vs host)

69582a3

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Nov 29, 2018

dns: fix inconsistent (hostname vs host)

f928d99

Fixes: #20892 PR-URL: #23572 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

BethGriggs added the backported-to-v10.x label Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Solve fix inconsistent (hostname vs host) in DNS. #20892#23572

Solve fix inconsistent (hostname vs host) in DNS. #20892#23572
shakeelmohamed wants to merge 5 commits intonodejs:masterfrom
shakeelmohamed:20892

shakeelmohamed commented Oct 12, 2018 •

edited

Loading

Uh oh!

shakeelmohamed commented Oct 14, 2018

Uh oh!

lundibundi Oct 14, 2018

Uh oh!

addaleax Oct 15, 2018

Uh oh!

lundibundi Oct 16, 2018 •

edited

Loading

Uh oh!

addaleax commented Oct 16, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

addaleax commented Oct 21, 2018

Uh oh!

Trott commented Oct 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

shakeelmohamed commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shakeelmohamed commented Oct 14, 2018

Uh oh!

lundibundi Oct 14, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

lundibundi Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 16, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

addaleax commented Oct 21, 2018

Uh oh!

Trott commented Oct 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

shakeelmohamed commented Oct 12, 2018 •

edited

Loading

lundibundi Oct 16, 2018 •

edited

Loading