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: update source, related test & doc for ArrayBuffer/DataView #23210

Closed
wants to merge 2 commits into from
Closed

tls: update source, related test & doc for ArrayBuffer/DataView #23210

wants to merge 2 commits into from

Conversation

BeniCheni
Copy link
Contributor

Refs: #1826

Referring to the comment in "mentor-available" #1826, this PR tries out the tls item from the checklist of the comment: (Thanks for your review!)

  • use isArrayBufferView in place of isUint8Array
  • update test code in "test-tls-basic-validations.js"
  • update tls.md doc.
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Oct 2, 2018
@BeniCheni BeniCheni mentioned this pull request Oct 2, 2018
Closed
vsemozhetbyt
vsemozhetbyt reviewed Oct 2, 2018
View reviewed changes
doc/api/tls.md Outdated
first byte is the length of the next protocol name. Passing an array is
usually much simpler, e.g. `['hello', 'world']`.
* `ALPNProtocols`: {string[]|Buffer[]|TypedArray[]|DataView[]|Buffer|
TypedArray|DataView}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it seems this line needs 2 more spaces for indent.

doc/api/tls.md Outdated
usually much simpler, e.g. `['hello', 'world']`.
(Protocols should be ordered by their priority.)
* `ALPNProtocols`: {string[]|Buffer[]|TypedArray[]|DataView[]|Buffer|
TypedArray|DataView}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same.

@BeniCheni
Copy link
Contributor Author

@vsemozhetbyt ,
Updated the indentation in tls.md for both L889 and L1144 per previous comments. Please review again.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 2, 2018 •
edited
Loading

Doc changes LGTM)

CI: https://ci.nodejs.org/job/node-test-pull-request/17591/

@lundibundi
Copy link
Member

lundibundi commented Oct 3, 2018 •
edited
Loading

ping @BeniCheni could you rebase this on master, please?
Misread the time 🙃

test.sequential/test-gc-net-timeout timeouted

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/17599/

@joyeecheung joyeecheung mentioned this pull request Oct 3, 2018
Closed
@BeniCheni
tls: update source, related test & doc for ArrayBuffer/DataView
b81c6e5 
In tls module, accept ArrayBuffer/DataView in place of isUint8Array in
the source code & related test code in "test-tls-basic-validations.js",
per the "tls" item in the checklist of the comment in #1826.

Refs: #1826 (comment)
@BeniCheni
doc: update tls.md with indentation per linting & PR review
b237ebd
@BeniCheni
Copy link
Contributor Author

🙏 for reviewing, approving & the CI instance, @lundibundi. Minor FYI I saw a previous CI flaky failure, so I just rebased & pushed to get "green" CI again.

@BeniCheni
Copy link
Contributor Author

Ping / dear reviewers,

Friendly reminder the previous approvals & “green” CI are all set for this PR, ready for “author ready”. Hoping the PR wouldn’t become too stale. Thanks.

@lundibundi
Copy link
Member

Sorry for the delay, forgot to start CI after the first message.
https://ci.nodejs.org/job/node-test-pull-request/17667/

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 6, 2018
@danbev
Copy link
Contributor

danbev commented Oct 8, 2018

Landed in f5ab9d1.

@danbev danbev closed this Oct 8, 2018
danbev pushed a commit that referenced this pull request Oct 8, 2018
@BeniCheni @danbev
tls: update test & docs for ArrayBuffer/DataView
f5ab9d1 
In tls module, accept ArrayBuffer/DataView in place of isUint8Array in
the source code & related test code in "test-tls-basic-validations.js",
per the "tls" item in the checklist of the comment in #1826.

PR-URL: #23210
Refs: #1826
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@BeniCheni BeniCheni deleted the tls-array-buffer-view branch October 8, 2018 14:35
@targos
Copy link
Member

targos commented Oct 10, 2018

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

jasnell pushed a commit that referenced this pull request Oct 17, 2018
@BeniCheni @jasnell
tls: update test & docs for ArrayBuffer/DataView
dc33b3e 
In tls module, accept ArrayBuffer/DataView in place of isUint8Array in
the source code & related test code in "test-tls-basic-validations.js",
per the "tls" item in the checklist of the comment in #1826.

PR-URL: #23210
Refs: #1826
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@jasnell jasnell jasnell approved these changes

@lundibundi lundibundi lundibundi approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
7 participants
@BeniCheni @vsemozhetbyt @lundibundi @danbev @targos @jasnell @nodejs-github-bot