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

src, doc: remove SSLv2 constants and descriptions #5541

Closed
wants to merge 1 commit into from
Closed

src, doc: remove SSLv2 constants and descriptions #5541

wants to merge 1 commit into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Mar 3, 2016

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][0]?
  • 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)?

Affected core subsystem(s)

src, doc

Description of change

Constants and doc descriptions related to SSLv2 are no longer needed.

Fixes: #5529

CI of https://ci.nodejs.org/job/node-test-commit/2435/ is fine except existing lint errors and flaky tests on Win.

R= @bnoordhuis , @rvagg

src, doc: remove SSLv2 constants and descriptions
d60a061 
Constants and doc descriptions related to SSLv2 are no longer needed.

Fixes: nodejs#5529
@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. labels Mar 3, 2016
bnoordhuis
bnoordhuis reviewed Mar 3, 2016
View reviewed changes
doc/api/tls.markdown
`--enable-ssl3` flag respectively. In future versions of Node.js SSLv2 and
SSLv3 will not be compiled in by default.
If you wish to enable SSLv3, run node with the `--enable-ssl3` flag
respectively. In future versions of Node.js SSLv3 will not be compiled in by
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the 'respectively' here.

@bnoordhuis
Copy link
Member

Documentation changes LGTM with a nit (and good catch, I forgot about that).

I left node_constants.cc unchanged on purpose although I guess removing the constants shouldn't hurt.

@rvagg
Copy link
Member

rvagg commented Mar 3, 2016

The constants changes should be done on master first rather than bottom-up like this.

@bnoordhuis it's past midnight in Tokyo, any objection to me cherry-picking this and backing out the constants changes to get v0.10 out?

@shigeki
Copy link
Contributor Author

shigeki commented Mar 3, 2016

The fix has already been made. I' m about to land it.

shigeki pushed a commit that referenced this pull request Mar 3, 2016
doc: remove SSLv2 descriptions
6db377b 
Doc descriptions related to SSLv2 are no longer needed.

Fixes: #5529
PR-URL: #5541
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki
Copy link
Contributor Author

shigeki commented Mar 3, 2016

Fixed for only doc change and Ben's comment. Landed in 6db377b. Please go a head of release v0.10.

@shigeki shigeki closed this Mar 3, 2016
@rvagg
Copy link
Member

rvagg commented Mar 3, 2016

woo! dediation, thanks a lot @shigeki, you should probably sleep now.

@shigeki
Copy link
Contributor Author

shigeki commented Mar 3, 2016

@rvagg Thanks. You too take care of yourself.

rvagg pushed a commit that referenced this pull request Mar 3, 2016
@rvagg
doc: remove SSLv2 descriptions
ce58c2c 
Doc descriptions related to SSLv2 are no longer needed.

Fixes: #5529
PR-URL: #5541
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Mar 3, 2016
Merged
@rvagg
Copy link
Member

rvagg commented Mar 3, 2016

added to v0.12 @ ce58c2c

jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
@jBarz
doc: remove SSLv2 descriptions
c9b206f 
Doc descriptions related to SSLv2 are no longer needed.

Fixes: nodejs/node#5529
PR-URL: nodejs/node#5541
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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
c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shigeki @bnoordhuis @rvagg @mscdex