Skip to content
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

tls: remove tls_legacy, createSecurePair #5924

Closed
wants to merge 1 commit into from

Conversation

jhamhader
Copy link
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 mscdex added the tls Issues and PRs related to the tls subsystem. label Mar 27, 2016
@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2016

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

This is a semver-major change.
Remove SecurePair, createSecure pair from API.
Remove internal Connection class and SecurePair tests.
@jhamhader
Copy link
Contributor Author

  1. I pushed that after an IRC talk with @indutny
  2. Resolving conflicts in a few seconds

@jasnell
Copy link
Member

jasnell commented Mar 27, 2016

A deprecation cycle would definitely be required first.

@indutny
Copy link
Member

indutny commented Mar 27, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2016

@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
Copy link
Member

indutny commented Mar 27, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Mar 27, 2016

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

@indutny
Copy link
Member

indutny commented Mar 27, 2016

@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
Copy link
Contributor Author

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

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 1, 2016
@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

Closing in favor of #11349

@jasnell jasnell closed this Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants