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

[v10.x] doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION #28827

Closed
wants to merge 2 commits into from
Closed

[v10.x] doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION #28827

wants to merge 2 commits into from

Conversation

ckarande
Copy link
Contributor

Add documentation for tls.DEFAULT_MAX_VERSION and
tls.DEFAULT_MIN_VERSION, which existed in v10.6.0

Fixes: #28758
Refs: #26821

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

@ckarande
doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION
e3ebd51 
Add documentation for tls.DEFAULT_MAX_VERSION and
tls.DEFAULT_MIN_VERSION, which existed in v10.6.0

Fixes: nodejs#28758
Refs: nodejs#26821
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. v10.x labels Jul 23, 2019
@addaleax addaleax changed the title doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION [v10.x] doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION Jul 23, 2019
addaleax
addaleax approved these changes Jul 23, 2019
View reviewed changes
doc/api/cli.md Outdated
@@ -773,6 +773,8 @@ greater than `4` (its current default value). For more information, see the
[`Buffer`]: buffer.html#buffer_class_buffer
[`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`tls.DEFAULT_MAX_VERSION`]: tls.html#tls_tls_default_max_version
[`tls.DEFAULT_MIN_VERSION`]: tls.html#tls_tls_default_min_version
Copy link
Member

Choose a reason for hiding this comment

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

I feel like these should be added to the other file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I back-ported this commit from cli.md to 10.x. However, if it is needed in other file (tls.md) I can certainly move/add it there. Do you have any suggestions on where to include it in the tls.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On further analysis I realized as the cli options (--tls-max-vXX, --tls-min-vXX) are not supported in v10, there are no corresponding sections for command line option in this file. So these changes do not need to be back ported. I will revert these lines. Thanks.

doc/api/tls.md Outdated
@@ -1198,12 +1198,12 @@ changes:
`object.passphrase` if provided, or `options.passphrase` if it is not.
* `maxVersion` {string} Optionally set the maximum TLS version to allow. One
of `TLSv1.2'`, `'TLSv1.1'`, or `'TLSv1'`. Cannot be specified along with the
`secureProtocol` option, use one or the other. **Default:** `'TLSv1.2'`.
`secureProtocol` option, use one or the other. **Default:** [`tls.DEFAULT_MAX_VERSION`][].
Copy link
Member

Choose a reason for hiding this comment

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

I’d expect the linter to complain about the line length here, so:

Suggested change
`secureProtocol` option, use one or the other. **Default:** [`tls.DEFAULT_MAX_VERSION`][].
`secureProtocol` option, use one or the other.
**Default:** [`tls.DEFAULT_MAX_VERSION`][].

@nitinsurana
Copy link

nitinsurana commented Jul 24, 2019 •
edited

Adding

`[`tls.DEFAULT_MAX_VERSION`]: #tls_tls_default_max_version`
`[`tls.DEFAULT_MIN_VERSION`]: #tls_tls_defaut_min_version`

right after

`[`tls.DEFAULT_ECDH_CURVE`]: #tls_tls_default_ecdh_curve`

in tls.md

should fix the build failure.

@ckarande
Copy link
Contributor Author

Thanks @nitinsurana @addaleax . The lint error was coming from cli.md due to unused links. Also, these links were needed in tls.md. I have fixed it in the latest commit.

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jul 26, 2019

I think the crashes on CI will be fixed when #25079 is backported to the v10.x-staging branch. In the meantime, hopefully Resume Build once or twice will fix....

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jul 26, 2019

I think the crashes on CI will be fixed when #25079 is backported to the v10.x-staging branch. In the meantime, hopefully Resume Build once or twice will fix....

Backport is in #28832. After that lands, CI here should be green or at least green-ish-er.

@Trott
Copy link
Member

Trott commented Aug 2, 2019

I think the crashes on CI will be fixed when #25079 is backported to the v10.x-staging branch. In the meantime, hopefully Resume Build once or twice will fix....

Backport is in #28832. After that lands, CI here should be green or at least green-ish-er.

Fix for the crash has landed. Time to launch CI again....

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

BethGriggs pushed a commit that referenced this pull request Oct 7, 2019
@ckarande @BethGriggs
doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION
cef5010 
Add documentation for tls.DEFAULT_MAX_VERSION and
tls.DEFAULT_MIN_VERSION, which existed in v10.6.0

Fixes: #28758
Refs: #26821

PR-URL: #28827
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Oct 7, 2019
@ckarande @BethGriggs
doc: fix the links tls default version sections
c285e69 
PR-URL: #28827
Fixes: #28758
Refs: #26821
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs
Copy link
Member

Landed in c285e69
Thank you @ckarande

@BethGriggs BethGriggs closed this Oct 7, 2019
@BethGriggs BethGriggs mentioned this pull request Oct 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ckarande @nitinsurana @nodejs-github-bot @Trott @BethGriggs @addaleax