Skip to content

tls: rename validateKeyCert in _tls_common.js - #28116

Closed
danbev wants to merge 2 commits into
nodejs:masterfrom
danbev:validateKeyCertArg
Closed

tls: rename validateKeyCert in _tls_common.js#28116
danbev wants to merge 2 commits into
nodejs:masterfrom
danbev:validateKeyCertArg

Conversation

@danbev

Copy link
Copy Markdown
Contributor

This commit renames validateKeyCert to validateKeyCertArg to avoid
confusing this with something that would validate the actual key or
certificate.

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

@nodejs-github-bot

nodejs-github-bot commented Jun 7, 2019

Copy link
Copy Markdown
Collaborator

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23843/

@nodejs-github-botnodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jun 7, 2019
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mscdex

Copy link
Copy Markdown
Contributor

s/_tls_commons/_tls_common/ in commit message

@mscdex

Copy link
Copy Markdown
Contributor

Also I think the subsystem prefix should be tls: instead of crypto:.

This commit renames validateKeyCert to validateKeyCertArg to avoid
confusing this with something that would validate the actual key or
certificate.
@danbev
danbevforce-pushed the validateKeyCertArg branch from 2b85de2 to d048ddbCompareJune 7, 2019 06:45
@danbev

Copy link
Copy Markdown
ContributorAuthor

@mscdex Thanks, I've fixed both issues now.

@danbevdanbev changed the title crypto: rename validateKeyCert in _tls_commons.jstls: rename validateKeyCert in _tls_common.jsJun 7, 2019
@sam-github

Copy link
Copy Markdown
Contributor

I'm fine with this, as-is, but maybe validateKeyOrCertOption() would be more accurate? It's not being validated that its the key's cert, and its passed as an option, not an arg (though the error is ERR_INVALID_ARG_TYPE).

@danbev

Copy link
Copy Markdown
ContributorAuthor

I'm fine with this, as-is, but maybe validateKeyOrCertOption() would be more accurate?

I like that better. I've updated with a commit now. Thanks

@nodejs-github-bot

This comment has been minimized.

@danbevdanbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 12, 2019
@Trott

Copy link
Copy Markdown
Member

@Trott

Copy link
Copy Markdown
Member

Landed in 173bee2

@TrottTrott closed this Jun 13, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 13, 2019
This commit renames validateKeyCert to validateKeyCertArg to avoid
confusing this with something that would validate the actual key or
certificate.
PR-URL: nodejs#28116
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
This commit renames validateKeyCert to validateKeyCertArg to avoid
confusing this with something that would validate the actual key or
certificate.
PR-URL: #28116
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeARBridgeAR mentioned this pull request Jun 17, 2019
@danbev
danbev deleted the validateKeyCertArg branch August 20, 2019 07:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@danbev@nodejs-github-bot@mscdex@sam-github@Trott@jasnell@antsmartian@lpinca@cjihrig