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

tls addContext should check typeof context #34442

Closed
wants to merge 6 commits into from
Closed

tls addContext should check typeof context #34442

wants to merge 6 commits into from

Conversation

masx200
Copy link
Contributor

@masx200 masx200 commented Jul 20, 2020

Checklist
  • [ x] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [ x] tests and/or benchmarks are included
  • [x ] documentation is changed or added
  • [ x] commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jul 20, 2020
@masx200 masx200 mentioned this pull request Jul 20, 2020
Closed
addaleax
addaleax reviewed Jul 20, 2020
View reviewed changes
lib/_tls_wrap.js Outdated Show resolved Hide resolved
addaleax
addaleax previously approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This seems good, up to the linter failures

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 20, 2020
@masx200
Copy link
Contributor Author

masx200 commented Jul 20, 2020

#34110

servername should also be case-insensitive.

addaleax
addaleax reviewed Jul 20, 2020
View reviewed changes
lib/_tls_wrap.js Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Jul 20, 2020
View reviewed changes
lib/_tls_wrap.js Outdated Show resolved Hide resolved
@masx200 @jasnell
Update lib/_tls_wrap.js
363fb7f 
Co-authored-by: James M Snell <jasnell@gmail.com>
rickyes
rickyes reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@rickyes rickyes left a comment

Choose a reason for hiding this comment

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

It might be better to add test cases.

@masx200
Copy link
Contributor Author

masx200 commented Jul 21, 2020

I don’t know much about writing tests. Can you ask someone to write tests?

@aduh95
Copy link
Contributor

aduh95 commented Nov 8, 2020

@masx200 can you fix the linter errors please?

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2020

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2021

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickyes rickyes rickyes left review comments

@addaleax addaleax addaleax left review comments

@jasnell jasnell jasnell left review comments

Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled. 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.
6 participants
@masx200 @aduh95 @jasnell @addaleax @rickyes @nodejs-github-bot