FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: remove _optional_ designation for tls options by Trott · Pull Request #22545 · nodejs/node · GitHub

/ node Public

doc: remove _optional_ designation for tls options - #22545

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:optional
ConversationCommits (2)ChecksFiles changed
Closed

doc: remove _optional_ designation for tls options#22545
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:optional

Conversation

Trott commented Aug 27, 2018

Copy link
Copy Markdown
Member

Options are, by definition, optional. Remove specification of some
options as "optional".

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

doc: remove _optional_ designation for tls options
90bc491 
Options are, by definition, optional. Remove specification of some
options as "optional".

nodejs-github-bot commented Aug 27, 2018

Copy link
Copy Markdown
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/689/pipeline

nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Aug 27, 2018

vsemozhetbyt commented Aug 27, 2018

Copy link
Copy Markdown
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

vsemozhetbyt added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 27, 2018
lpinca reviewed Aug 27, 2018
View reviewed changes
Comment thread doc/api/tls.md
provide the client certificate.
* `crl` {string|string[]|Buffer|Buffer[]} Optional PEM formatted
CRLs (Certificate Revocation Lists).
* `cert` {string|string[]|Buffer|Buffer[]} Cert chains in PEM format. One cert

lpinca Aug 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure this is a good idea. It's hard to tell with options can be omitted now. It seems they must all be specified now.

Trott Aug 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I'm not mistaken, none of them are required. (If there were required things, it should probably be named config rather than options.) I think that by specifying "Optional" for some options, we mislead the reader into thinking that some options are required.

We have it called with an empty options object twice here.

I do think the function signature needs to be updated to put options in [] to make it clear that it can be omitted, like it is here and here.

Trott Aug 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I do think the function signature needs to be updated

I've now done this in a separate commit. @vsemozhetbyt PTAL to confirm you're still 👍 on this with that addition. Thanks!

vsemozhetbyt Aug 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but maybe let's get at least one LGTM from @nodejs/crypto to be on the safe side.

Trott Aug 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, but maybe let's get at least one LGTM from @nodejs/crypto to be on the safe side.

I'll remove the fast-track label, at least until that happens. No need to rush this.

Trott Aug 31, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@nodejs/crypto Any chance someone can take a quick look at this?

Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 27, 2018
doc: indicate createSecureContext arg is optional
d92be8e 
The `options` argument to `tls.createSecureContext()` is optional.

Indicate this by using `[` and `]` in the function signature.
lpinca approved these changes Aug 27, 2018
View reviewed changes

lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM now.

Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label Aug 27, 2018

Trott commented Aug 31, 2018

Copy link
Copy Markdown
Member Author

Probably overkill to do this but just in case folks there would want to look this over: /ping @nodejs/security-wg

lirantal commented Aug 31, 2018

Copy link
Copy Markdown
Member

LGTM @Trott with only a small concern that as @lpinca pointed out it might be misleading with regards to what is optional or not. In the future someone may refactor this code and rename options with data or something else and then the variable naming won't be that obvious.

I'm suggesting maybe we can specify in a comment that all options variables are indeed optional.

lpinca commented Aug 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@lirantal my comment was addressed by making it optional in the signature. I think that is sufficient.
See d92be8e.

lirantal commented Aug 31, 2018

Copy link
Copy Markdown
Member

yep, didn't notice that and makes sense 👍

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 2, 2018

addaleax commented Sep 2, 2018

Copy link
Copy Markdown
Member

Landed in 1672484, 8452459

addaleax closed this Sep 2, 2018
addaleax pushed a commit that referenced this pull request Sep 2, 2018
doc: remove _optional_ designation for tls options
1672484 
Options are, by definition, optional. Remove specification of some
options as "optional".

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Sep 2, 2018
doc: indicate createSecureContext arg is optional
8452459 
The `options` argument to `tls.createSecureContext()` is optional.

Indicate this by using `[` and `]` in the function signature.

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 2, 2018
doc: remove _optional_ designation for tls options
4c60f49 
Options are, by definition, optional. Remove specification of some
options as "optional".

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 2, 2018
doc: indicate createSecureContext arg is optional
a525ab0 
The `options` argument to `tls.createSecureContext()` is optional.

Indicate this by using `[` and `]` in the function signature.

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
doc: remove _optional_ designation for tls options
0e862da 
Options are, by definition, optional. Remove specification of some
options as "optional".

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
doc: indicate createSecureContext arg is optional
a4545ad 
The `options` argument to `tls.createSecureContext()` is optional.

Indicate this by using `[` and `]` in the function signature.

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
doc: remove _optional_ designation for tls options
68f7f87 
Options are, by definition, optional. Remove specification of some
options as "optional".

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
doc: indicate createSecureContext arg is optional
672f74e 
The `options` argument to `tls.createSecureContext()` is optional.

Indicate this by using `[` and `]` in the function signature.

PR-URL: #22545
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
This was referenced Sep 6, 2018
Closed
Closed
Trott deleted the optional branch January 13, 2022 22:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
trivikr trivikr approved these changes
+2 more reviewers
vsemozhetbyt vsemozhetbyt approved these changes
gdams gdams approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL