★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18335Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

crypto: docs-only deprecate crypto.fips, replace #18335

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@ChALkeR ChALkeR

@Trott Trott

@vsemozhetbyt vsemozhetbyt

@mcollina mcollina

@guybedford guybedford

@addaleax addaleax

@cjihrig cjihrig

@maclover7 maclover7

@BridgeAR BridgeAR

@mhdawson mhdawson

Assignees
No one assigned
Labels
crypto dont-land-on-v8.x semver-minor
Projects
None yet
Milestone
  10.0.0
@jasnell
Copy link
Member

jasnell commented Jan 24, 2018

Docs-only deprecate the getter/setter crypto.fips and replace with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support
Refs: #18131

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)

crypto

@jasnell jasnell added crypto semver-major labels Jan 24, 2018

@nodejs-github-bot nodejs-github-bot added the crypto label Jan 24, 2018

@jasnell jasnell requested a review from nodejs/tsc Jan 24, 2018

@Trott

Trott reviewed Jan 24, 2018
View changes

doc/api/deprecations.md Outdated
Type: Documentation-only
The [`crypto.fips`][] property has been deprecated and replaced with
`crypto.setFips()` and `crypto.getFips()`.

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Jan 24, 2018

Member

I prefer more direct wording:

The [`crypto.fips`][] property is deprecated. Please use
`crypto.setFips()` and `crypto.getFips()`.
@addaleax

addaleax approved these changes Jan 24, 2018
View changes

@jasnell jasnell added this to the 10.0.0 milestone Jan 24, 2018

@vsemozhetbyt

vsemozhetbyt reviewed Jan 24, 2018
View changes

doc/api/crypto.md Outdated
Returns `true` if (and only if) a FIPS compliant crypto provider is
currently in use.

This property is deprecated. Please use `crypto.setFips()` and

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Jan 24, 2018

Member

Typo sentence?
@vsemozhetbyt

vsemozhetbyt reviewed Jan 24, 2018
View changes

doc/api/deprecations.md Outdated
@@ -831,6 +839,7 @@ is not included in this list will be considered invalid in compliance with
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`crypto.createCredentials()`]: crypto.html#crypto_crypto_createcredentials_details
[`crypto.fips`]: crypto.html#crypto_crypto.fips

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Jan 24, 2018

Member

#crypto_crypto.fips -> #crypto_crypto_fips?
@cjihrig

cjihrig approved these changes Jan 24, 2018
View changes

doc/api/crypto.md Outdated
added: REPLACEME
-->

Returns `true` if (and only if) a FIPS compliant crypto provider is

This comment has been minimized.

Sign in to view
Copy link
@cjihrig

cjihrig Jan 24, 2018

Contributor

No need for the parentheses.
@mcollina

mcollina approved these changes Jan 24, 2018
View changes

Copy link
Member

mcollina left a comment

LGTM
@guybedford

guybedford approved these changes Jan 24, 2018
View changes

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Jan 24, 2018

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Jan 26, 2018

Ping @nodejs/tsc ... I'd like to treat this docs-only deprecation as semver-minor. Any objections?
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Jan 26, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/12759/
@maclover7

maclover7 approved these changes Jan 26, 2018
View changes

@maclover7 maclover7 added the author ready label Jan 26, 2018

@maclover7 maclover7 force-pushed the nodejs:master branch from bb5575a to 993b716 Jan 26, 2018

@cjihrig cjihrig force-pushed the nodejs:master branch from 993b716 to 082f952 Jan 26, 2018

@ChALkeR

This comment has been minimized.

Sign in to view
Copy link
Member

ChALkeR commented Jan 28, 2018 •
edited

+1 for semver-minor for doc-only here.

Perhaps there should also be a pendingDeprecation-guarded runtime warning?

Upd: opened #18417.
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Feb 1, 2018

@ChALkeR ... let's add the --pending-deprecation warning after #18417 lands. This shouldn't have to wait for that tho.
@ChALkeR

This comment has been minimized.

Sign in to view
Copy link
Member

ChALkeR commented Feb 1, 2018

@jasnell #18417 is an issue, #18433 landed already.

@jasnell jasnell force-pushed the jasnell:crypto-replace-crypto.fips branch from 54bd37a to 877e1c9 Feb 1, 2018

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Feb 1, 2018

oh! I missed that... think I mentally mixed #18433 and #18417 and completely missed that it had landed already :-)
@ChALkeR

ChALkeR reviewed Feb 1, 2018
View changes

doc/api/deprecations.md Outdated
@@ -845,7 +852,15 @@ The [`crypto.DEFAULT_ENCODING`][] property is deprecated.
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`crypto.createCredentials()`]: crypto.html#crypto_crypto_createcredentials_details
<<<<<<< HEAD
<<<<<<< HEAD

This comment has been minimized.

Sign in to view
Copy link
@ChALkeR

ChALkeR Feb 1, 2018 •
edited

Member

Merge conflict (here and below)

This comment has been minimized.

Sign in to view
Copy link
@jasnell

jasnell Feb 1, 2018

Author Member

Ugh. Forgot to save

@BridgeAR BridgeAR added semver-minor and removed author ready semver-major labels Feb 1, 2018

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented Feb 1, 2018

I changed it to semver-minor as there was enough time to speak up against it and there was only a voice pro.
@mhdawson

mhdawson approved these changes Feb 1, 2018
View changes

Copy link
Member

mhdawson left a comment

LGTM

@jasnell jasnell force-pushed the jasnell:crypto-replace-crypto.fips branch from d488810 to 900bf45 Feb 1, 2018

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Feb 1, 2018

Note: I will land this after I get #18492 landed.
@BridgeAR

BridgeAR approved these changes Feb 1, 2018
View changes

@BridgeAR BridgeAR added the author ready label Feb 1, 2018

@jasnell
crypto: docs-only deprecate crypto.fips, replace 
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support
Refs: #18131
Loading status checks…
1dece49

@jasnell jasnell force-pushed the jasnell:crypto-replace-crypto.fips branch from 900bf45 to 1dece49 Feb 1, 2018

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Feb 1, 2018

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

jasnell added a commit that referenced this pull request Feb 2, 2018

@jasnell
crypto: docs-only deprecate crypto.fips, replace 
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: #18335
Refs: #18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
6e7992e
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

jasnell commented Feb 2, 2018

Landed in 6e7992e

@jasnell jasnell closed this Feb 2, 2018

@addaleax addaleax removed the author ready label Feb 4, 2018

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

@jasnell @msoechting
crypto: docs-only deprecate crypto.fips, replace 
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: nodejs#18335
Refs: nodejs#18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
1fa463b

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

@jasnell @msoechting
crypto: docs-only deprecate crypto.fips, replace 
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: nodejs#18335
Refs: nodejs#18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
2bbab17

@MylesBorins MylesBorins added the backport-requested-v9.x label Feb 20, 2018

@MylesBorins

This comment has been minimized.

Sign in to view
Copy link
Member

MylesBorins commented Feb 20, 2018

This does not land cleanly on v9.x

This is because other deprecations have landed on master that have been given codes. If we are going to land this as semver minor would someone be willing to backport and assign an appropriate code to make sure we don't double dip

@devsnek devsnek referenced this pull request Mar 15, 2018

Closed

events: deprecate EventEmitter.defaultMaxListeners getter/setter #18712

4 of 4 tasks complete

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

@jasnell @BridgeAR
crypto: docs-only deprecate crypto.fips, replace 
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: nodejs#18335
Refs: nodejs#18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
da3cc16

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

@jasnell @MayaLekova
crypto: docs-only deprecate crypto.fips, replace 
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: nodejs#18335
Refs: nodejs#18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
064c937

@MylesBorins MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.