GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tls: remove tls_legacy, createSecurePair by jhamhader · Pull Request #5924 · nodejs/node

Conversation

@jhamhader

@jhamhader 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Mar 27, 2016
@mscdex

mscdex commented Mar 27, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jhamhader
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 jhamhader mentioned this pull request Mar 27, 2016
Fix rejectUnauthorized: undefined in tls.connect, requestCert in tls.createServer #5917
Closed
2 tasks
@jhamhader

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jhamhader
jhamhader force-pushed the remove-tls-legacy branch from 76c28bc to 14b8e5a Compare March 27, 2016 14:48
@jasnell

jasnell commented Mar 27, 2016

Copy link
Copy Markdown
Member

A deprecation cycle would definitely be required first.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

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...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Mar 27, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jhamhader

jhamhader commented Mar 27, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 1, 2016
@jhamhader jhamhader mentioned this pull request Apr 5, 2016
doc, tls: deprecate createSecurePair #6063
Closed
1 of 4 tasks
@estliberitas
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
@jhamhader jhamhader mentioned this pull request Jun 17, 2016
SNICallback doesn't appear to work with tls.createSecurePair() #4878
Closed
@Trott
Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09
@rvagg
rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@MylesBorins
MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00
@jasnell

jasnell commented Mar 1, 2017

Copy link
Copy Markdown
Member

Closing in favor of #11349
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@jhamhader @mscdex @jasnell @indutny
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.