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

tls: remove tls_legacy, createSecurePair by jhamhader · Pull Request #5924 · nodejs/node · GitHub

/ node Public

tls: remove tls_legacy, createSecurePair - #5924

Closed
jhamhader wants to merge 1 commit into
nodejs:masterfrom
jhamhader:remove-tls-legacy
ConversationCommits (1)ChecksFiles changed
Closed

tls: remove tls_legacy, createSecurePair#5924
jhamhader wants to merge 1 commit into
nodejs:masterfrom
jhamhader:remove-tls-legacy

Conversation

jhamhader commented Mar 27, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Please provide a description of the change here.

This is a semver-major change.
Remove SecurePair, createSecure pair from API.
Remove internal Connection class and SecurePair tests.

mscdex added the tls Issues and PRs related to the tls subsystem. label Mar 27, 2016

mscdex commented Mar 27, 2016

Copy link
Copy Markdown
Contributor

I don't think this can be removed without some sort of deprecation cycle.

tls: remove tls_legacy, createSecurePair
14b8e5a 
This is a semver-major change.
Remove SecurePair, createSecure pair from API.
Remove internal Connection class and SecurePair tests.
jhamhader mentioned this pull request Mar 27, 2016
Closed
2 tasks

jhamhader commented Mar 27, 2016

Copy link
Copy Markdown
Contributor Author
  1. I pushed that after an IRC talk with @indutny
  2. Resolving conflicts in a few seconds

jasnell commented Mar 27, 2016

Copy link
Copy Markdown
Member

A deprecation cycle would definitely be required first.

indutny commented Mar 27, 2016

Copy link
Copy Markdown
Member

@jasnell what exactly is missing? I believe createSecurePair had a deprecation warning in documentation for quite a long time now...

mscdex commented Mar 27, 2016

Copy link
Copy Markdown
Contributor

@indutny I think it needs to be deprecated at the code level via internal/util.deprecate() and not just documentation. Also, tls.createSecurePair() isn't even deprecated in the documentation.

indutny commented Mar 27, 2016

Copy link
Copy Markdown
Member

@mscdex perhaps there is a bug in docs, but it is deprecated: https://nodejs.org/api/tls.html#tls_class_cryptostream

mscdex commented Mar 27, 2016

Copy link
Copy Markdown
Contributor

@indutny There is no connection between CryptoStream and createSecurePair()/SecurePair in the docs.

indutny commented Mar 27, 2016

Copy link
Copy Markdown
Member

@mscdex yeah, looks like we will need to deprecate it first then. 😢

@jhamhader sorry, but this PR will have to wait a release cycle

jhamhader commented Mar 27, 2016

Copy link
Copy Markdown
Contributor Author

Will submit a deprecation PR and resume effort on SNI callback issue.
Thanks

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 1, 2016
jhamhader mentioned this pull request Apr 5, 2016
Closed
4 tasks
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
jhamhader mentioned this pull request Jun 17, 2016
Closed
rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

jasnell commented Mar 1, 2017

Copy link
Copy Markdown
Member

Closing in favor of #11349

jasnell closed this Mar 1, 2017
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

No reviews

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL