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

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

Closed
wants to merge 5 commits into from

Conversation

shakeelmohamed
Copy link
Contributor

@shakeelmohamed shakeelmohamed commented Oct 12, 2018

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

@nodejs-github-bot nodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Oct 12, 2018
@shakeelmohamed shakeelmohamed changed the title 20892 Solve fix inconsistent (hostname vs host) in DNS. #20892 Oct 12, 2018
@targos targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@shakeelmohamed
Copy link
Contributor Author

@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?

@@ -175,12 +175,12 @@ function lookupService(host, port, callback) {

var req = new GetNameInfoReqWrap();
req.callback = callback;
req.host = host;
req.hostname = hostname;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@lundibundi lundibundi Oct 16, 2018

Choose a reason for hiding this comment

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

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 =).

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
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]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 15, 2018
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
Copy link
Member

@addaleax 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
jasnell pushed a commit that referenced this pull request Oct 17, 2018
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
Copy link
Member

Trott commented Oct 18, 2018

@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
Copy link
Member

Trott commented Oct 18, 2018

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

Trott added a commit to Trott/io.js that referenced this pull request Oct 18, 2018
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
Copy link
Member

Trott commented Oct 18, 2018

refack pushed a commit that referenced this pull request Oct 18, 2018
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 pushed a commit that referenced this pull request Oct 20, 2018
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
Copy link
Member

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 21, 2018
Fixes: nodejs#20892
PR-URL: nodejs#23572
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 21, 2018

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/.)

@Trott Trott closed this Oct 21, 2018
jasnell pushed a commit that referenced this pull request Oct 21, 2018
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]>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
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 Oct 30, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
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 26, 2018
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]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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
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]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Fixes: #20892
PR-URL: #23572
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
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]>
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. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent documentation: hostname vs host
10 participants