FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove crypto.Credentials API by aduh95 · Pull Request #21153 · nodejs/node · GitHub

/ node Public

Remove crypto.Credentials API - #21153

Closed
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:fix-crypto-deprecation-warnings
ConversationCommits (1)ChecksFiles changed
Closed

Remove crypto.Credentials API#21153
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:fix-crypto-deprecation-warnings

Conversation

aduh95 commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

Fix #20793

The crypto.Credentials legacy API has been Runtime deprecated since
v0.11.13 and users had been adviced to use tls.SecureContext instead.

This PR removes it from the crypto module.

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

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jun 5, 2018
mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 5, 2018
targos reviewed Jun 5, 2018
View reviewed changes
Comment thread lib/crypto.js Outdated

targos Jun 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this change seems unrelated

targos commented Jun 5, 2018

Copy link
Copy Markdown
Member

@nodejs/crypto @nodejs/tsc

mcollina approved these changes Jun 5, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with the changes @targos mentioned

aduh95 force-pushed the fix-crypto-deprecation-warnings branch from 8163a85 to e0acbef Compare June 5, 2018 21:07
trivikr reviewed Jun 6, 2018
View reviewed changes
Comment thread doc/api/deprecations.md Outdated

trivikr Jun 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

has been removed -> was removed (ditto on line 155)
Similar to PR #21043

tniessen reviewed Jun 6, 2018
View reviewed changes
Comment thread lib/crypto.js Outdated

tniessen Jun 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The rest of the file does not use trailing commas, please remove this. I don't want to start any new discussions about trailing commas, but I'd go for consistency in this case.

ryzokuken Jun 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The line right above doesn't have a trailing comma either, but @bnoordhuis suggests we use trailing commas moving forwards?

Idk, sounds reasonable to just add them wherever you go.

tniessen Jun 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are you referring to #20703 (comment)? ESLint still doesn't have a file-by-file rule as far as I know, so that doesn't really apply here. Personally, I am not a fan of dangling commas, and we discussed that topic numerous times (e.g. #19131 (comment)). I totally understand Ben's reasoning, but this kind of "churn" isn't that common from my perspective, correct me if I am wrong.

Edit: Striked my own comment because I promised not to start a new debate about trailing commas.

ryzokuken Jun 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, let's not. We should probably skip the comma for consistency, you're right.

aduh95 force-pushed the fix-crypto-deprecation-warnings branch from e0acbef to 966bdf8 Compare June 6, 2018 17:51

Trott commented Jun 6, 2018

Copy link
Copy Markdown
Member

semver-major so needs two TSC approvals (already has one) + CITGM before landing

vsemozhetbyt reviewed Jun 6, 2018
View reviewed changes
Comment thread doc/api/deprecations.md Outdated

vsemozhetbyt Jun 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems this link will not be valid after this PR, so we need to delinkify `crypto.createCredentials()` here and remove the bottom reference.

vsemozhetbyt added the deprecations Issues and PRs related to deprecations. label Jun 6, 2018
aduh95 force-pushed the fix-crypto-deprecation-warnings branch from 966bdf8 to 29be62f Compare June 6, 2018 21:49
addaleax approved these changes Jun 7, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but only because of the existing runtime deprecation.

If we had had our process back then, this should just have gotten stuck at a documentation deprecation and a one-line alias.

vsemozhetbyt commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

@nodejs/crypto is this ready to land?

tniessen commented Jun 24, 2018

Copy link
Copy Markdown
Member

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

tniessen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 24, 2018
apapirovski removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 25, 2018

apapirovski commented Jun 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@aduh95 could you have a look at the test failures please?

tniessen commented Jun 25, 2018

Copy link
Copy Markdown
Member

You need to remove the Credentials line from test-crypto-classes.js.

aduh95 commented Jun 25, 2018

Copy link
Copy Markdown
Contributor Author

@tniessen Thank you for spotting that, I'll take care of that this evening (that is to say in ten hours from my time zone).

crypto: remove deprecated legacy API
f758e89 
The `crypto.Credentials` legacy API has been Runtime deprecated since
v0.11.13 and users had been adviced to use `tls.SecureContext` instead.

Fixes: nodejs#20793
aduh95 force-pushed the fix-crypto-deprecation-warnings branch from 29be62f to f758e89 Compare June 25, 2018 21:33

vsemozhetbyt commented Jun 26, 2018

Copy link
Copy Markdown
Contributor

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

mhdawson commented Jun 26, 2018

Copy link
Copy Markdown
Member

@shigeki any concerns ?

vsemozhetbyt commented Jul 9, 2018

Copy link
Copy Markdown
Contributor

@nodejs/crypto is this landable? Are there any concerns?

tniessen commented Jul 10, 2018

Copy link
Copy Markdown
Member

I am fine with this change, I don't know of any potential breakage or downsides.

tniessen mentioned this pull request Jul 12, 2018
Closed
2 tasks

vsemozhetbyt commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

Let's land this tomorrow if there are no objections then.

vsemozhetbyt commented Jul 13, 2018

Copy link
Copy Markdown
Contributor

Landed in d2ee7d6
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Jul 13, 2018
crypto: remove deprecated legacy API
d2ee7d6 
The `crypto.Credentials` legacy API has been Runtime deprecated since
v0.11.13 and users had been adviced to use `tls.SecureContext` instead.

PR-URL: #21153
Fixes: #20793
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Oct 2, 2018
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
tniessen tniessen approved these changes
trivikr trivikr approved these changes
+4 more reviewers
targos targos left review comments
vsemozhetbyt vsemozhetbyt left review comments
TimothyGu TimothyGu approved these changes
ryzokuken ryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. deprecations Issues and PRs related to deprecations. 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.

Back | FazBrowse Home | New Git URL