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.

Sign up for GitHub

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

Jump to bottom

doc: new TLSSocket has no secure context options #10545

Closed
wants to merge 1 commit into from
Closed

doc: new TLSSocket has no secure context options #10545

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Dec 30, 2016 •
edited
Loading

Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

Fix #10538

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc,tls

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Dec 30, 2016
@sam-github
doc: new TLSSocket has no secure context options
ee6421e 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

Fix nodejs#10538
@nbdd0121
Copy link

I believe that what describes in the doc is sensible. Maybe we should change the implementation instead?

@sam-github
Copy link
Contributor Author

First we doc it how it is, because it can't be changed in existing releases, changing would be semver major.

Then we can propose changing it for 8.x. And people will understand the change, because they will see the doc change, and be able to compare old behaviour to new.

@sam-github
Copy link
Contributor Author

PTAL @nodejs/documentation @nodejs/crypto

@benjamingr
Copy link
Member

Change LGTM, but I also find the behavior slightly confusing.

@sam-github
Copy link
Contributor Author

I agree, and I'm working on a PR to make the ctor pass its options to createSecureContext() to make it more consistent.

@addaleax addaleax mentioned this pull request Jan 5, 2017
Closed
@jasnell
Copy link
Member

jasnell commented Jan 5, 2017

@nodejs/crypto

mhdawson
mhdawson approved these changes Jan 5, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

indutny
indutny approved these changes Jan 5, 2017
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

addaleax commented Jan 6, 2017

I agree, and I'm working on a PR to make the ctor pass its options to createSecureContext() to make it more consistent.

Maybe it’s better to leave the Fix line in the commit message out then?

jasnell pushed a commit that referenced this pull request Jan 6, 2017
@sam-github @jasnell
doc: new TLSSocket has no secure context options
c1b12a2 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: #10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

Landed in c1b12a2. I removed the Fix line per @addaleax's suggestion

@jasnell jasnell closed this Jan 6, 2017
@sam-github sam-github deleted the tlssocket-ctx-correction branch January 9, 2017 19:11
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@sam-github @italoacasas
doc: new TLSSocket has no secure context options
3b4b262 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: nodejs#10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@sam-github @italoacasas
doc: new TLSSocket has no secure context options
9721ddf 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: nodejs#10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
@sam-github @italoacasas
doc: new TLSSocket has no secure context options
19f4a56 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: nodejs#10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@sam-github @italoacasas
doc: new TLSSocket has no secure context options
b70dde0 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: nodejs#10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
@MylesBorins
Copy link
Contributor

@sam-github should this be backported? If so please feel free to cherry-pick. If it doesn't land cleanly feel free to add do-not-land label.

@sam-github
Copy link
Contributor Author

It should backport to 6.x to fix b662de6, but not needed on 4.x. I will do.

MylesBorins pushed a commit that referenced this pull request Apr 13, 2017
@sam-github @MylesBorins
doc: new TLSSocket has no secure context options
3855504 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: #10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@sam-github @MylesBorins
doc: new TLSSocket has no secure context options
c138ba3 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: #10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@sam-github @andrew749
doc: new TLSSocket has no secure context options
ac0f60d 
Unlike all the other tls APIs, if any secure context configuration is
required, the caller is responsible for creating the context.

Corrects a doc regression introduced in caa7fa9.

PR-URL: nodejs/node#10545
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indutny indutny indutny approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. 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.
9 participants
@sam-github @nbdd0121 @benjamingr @jasnell @addaleax @MylesBorins @indutny @mhdawson @nodejs-github-bot