doc, tls: mark parseCertString() as deprecated by XadillaX · Pull Request #14245 · nodejs/node

XadillaX · 2017-07-15T03:18:44Z

tls.parseCertString() was made public by mistack. So mark it as
deprecated.

Refs: #14193
Refs: af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls, doc

XadillaX · 2017-07-15T03:19:03Z

This is a sub-PR of #14218 (comment).

Trott · 2017-07-15T04:03:53Z

/cc @jasnell

XadillaX · 2017-07-18T10:01:08Z

/ping @jasnell

addaleax · 2017-07-18T17:23:50Z

I assume this is blocked by #14218 to begin with

XadillaX · 2017-07-20T03:25:12Z

@addaleax Why it blocked?

And I've rebased the code. As @refack said, it's a sub-PR of #14218. I will split that PR into 3 sub-PRs and this is the first one of them.

@refack Did you mean:

mark the function as deprecated with documented-only

move the function and modify the document with runtime

move the constant

Yes, exactly (so that we could land just (1) in v8.x)

jasnell · 2017-07-20T04:52:56Z

@nodejs/ctc ... a decision needs to be made whether deprecating this in 8.x is appropriate.

Trott · 2017-07-20T05:13:37Z

doc/api/deprecations.md

Nit: Replace comma with a period. One then will begin a new sentence.

Trott · 2017-07-20T05:14:30Z

a decision needs to be made whether deprecating this in 8.x is appropriate.

Deprecations are supposed to be semver-major, right? We should stick to that unless there is a compelling user-focused argument otherwise.

Trott

LGTM with a nit.

jasnell · 2017-07-20T14:48:29Z

Yes, deprecations are semver-major, which means this PR would not be able to land as a backport to 8.x... unless we special case it as we're done in other cases. The question is whether this one rises to the level that a special case needs to be made. I'm not so sure. I signed off because the code change itself looks fine, but I'm not convinced that this should land.

Trott · 2017-07-20T18:26:03Z

I'm not convinced that this should land.

@jasnell You mean you're not convinced that it should land in 8.x but it's fine for 9.0.0? Or you mean not land at all?

Trott · 2017-07-20T18:27:37Z

Oh, never mind, I see, this is an 8.x-specific PR. Yeah, I'm not sure it should land either. Fine for 9.x...And I'm OK if CTC decides to special-case it, but that really really makes me wonder if we need to revise our deprecation policy because these special cases seem to come up more than I expect them to....

refack · 2017-07-21T12:09:27Z

Just repeating the observation that tls.parseCertString() is undocumented in v8.x so the options for 8 are:

Do nothing
Document
Document as deprecated

IMHO (3) is reasonable since it's definitely an edge case for deprecation, but not obviously semver-major. An academic argument could be made that since it's currently undocumented, it was "latently deprecated" all along, we are just documenting it now.

Trott · 2017-07-21T17:28:16Z

I wouldn't stand in the way of a doc-only deprecation. If we can get a clean diff for that, maybe we can ping CTC and take it as consensus if no one objects after 72 hours.

XadillaX · 2017-07-23T07:55:37Z

/ping @nodejs/ctc

Trott · 2017-07-23T16:28:15Z

@nodejs/ctc Any objections to landing this doc-only deprecation in 8.x? It is for a property that was made public by mistake.

Trott · 2017-07-23T16:29:29Z

@addaleax I'm not clear on why this would be blocked by #14218. Can you explain?

addaleax · 2017-07-23T18:35:34Z

@Trott It seems to me like if we introduce a deprecation in 8.x, we should also deprecate in master so that we don’t accidentally un-deprecate when releasing 9.0.0?

XadillaX · 2017-07-24T01:30:38Z

@addaleax I think we should land #14249 before releasing 9.0.

One is document-only and another is runtime.

XadillaX · 2017-09-01T07:25:20Z

I think this PR should be landed before #14249.

BridgeAR · 2017-09-02T07:51:03Z

If there are no objections from @nodejs/tsc I would land this as a semver-minor.

XadillaX · 2017-09-06T03:04:59Z

/ping @nodejs/tsc

jasnell · 2017-09-06T03:59:54Z

With no objections by now, I'd say this is likely good to go as a semver-minor

BridgeAR · 2017-09-08T02:37:46Z

Landed in 703a3b5

`tls.parseCertString()` was made public by mistack. So mark it as deprecated. PR-URL: #14245 Refs: #14193 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-09-10T02:48:27Z

Thi needs to be backported to v8.x-staging. You can follow the guide and raise a backport PR against v8.x-staging

`tls.parseCertString()` was made public by mistack. So mark it as deprecated. PR-URL: nodejs#14245 Refs: nodejs#14193 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`tls.parseCertString()` was made public by mistack. So mark it as deprecated. PR-URL: #14245 Refs: #14193 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](nodejs/node#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855) * Custom lookup functions are now supported. [#14560](nodejs/node#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)

nodejs-github-bot added doc Issues and PRs related to the documentations. v8.x labels Jul 15, 2017

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. blocked PRs that are blocked by other issues or PRs. labels Jul 18, 2017

addaleax force-pushed the v8.x-staging branch from 71c15d5 to e421f08 Compare July 18, 2017 18:48

XadillaX changed the title ~~doc,tls: mark parseCertString() as deprecated~~ [v8.x] doc,tls: mark parseCertString() as deprecated Jul 20, 2017

XadillaX force-pushed the mark-tls.parseCertString-as-deprecated branch from 4387926 to 1def55b Compare July 20, 2017 03:33

jasnell approved these changes Jul 20, 2017

View reviewed changes

Trott reviewed Jul 20, 2017

View reviewed changes

doc/api/deprecations.md Outdated

Copy link

Member

Trott Jul 20, 2017

Choose a reason for hiding this comment

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

Nit: Replace comma with a period. One then will begin a new sentence.

Trott approved these changes Jul 20, 2017

View reviewed changes

Fishrock123 force-pushed the v8.x-staging branch from e9491f1 to 7c3fed5 Compare July 20, 2017 16:00

XadillaX force-pushed the mark-tls.parseCertString-as-deprecated branch from b95abf4 to 1cb3e56 Compare July 23, 2017 07:54

Trott added the ctc-review label Jul 23, 2017

update after reviewing

66d7e59

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 6, 2017

BridgeAR closed this Sep 8, 2017

MylesBorins added the backport-requested-v8.x label Sep 10, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins removed the backport-requested-v8.x label Oct 7, 2017

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

Uh oh!

Conversation

XadillaX commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

XadillaX commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 15, 2017

Uh oh!

XadillaX commented Jul 18, 2017

Uh oh!

addaleax commented Jul 18, 2017

Uh oh!

XadillaX commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jul 20, 2017

Uh oh!

Trott Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Jul 20, 2017

Uh oh!

Trott left a comment • 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!

jasnell commented Jul 20, 2017

Uh oh!

Trott commented Jul 20, 2017

Uh oh!

Trott commented Jul 20, 2017

Uh oh!

refack commented Jul 21, 2017

Uh oh!

Trott commented Jul 21, 2017

Uh oh!

XadillaX commented Jul 23, 2017

Uh oh!

Trott commented Jul 23, 2017

Uh oh!

Trott commented Jul 23, 2017

Uh oh!

addaleax commented Jul 23, 2017

Uh oh!

XadillaX commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XadillaX commented Sep 1, 2017

Uh oh!

BridgeAR commented Sep 2, 2017

Uh oh!

XadillaX commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 6, 2017

Uh oh!

BridgeAR commented Sep 8, 2017

Uh oh!

MylesBorins commented Sep 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

XadillaX commented Jul 15, 2017 •

edited

Loading

XadillaX commented Jul 15, 2017 •

edited

Loading

XadillaX commented Jul 20, 2017 •

edited

Loading

Trott left a comment •

edited

Loading

XadillaX commented Jul 24, 2017 •

edited

Loading

XadillaX commented Sep 6, 2017 •

edited

Loading