Skip to content

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

Closed
ckarande wants to merge 2 commits intonodejs:v10.x-stagingfrom
ckarande:backport-26821-to-v10.x
Closed

[v10.x] doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION#28827
ckarande wants to merge 2 commits intonodejs:v10.x-stagingfrom
ckarande:backport-26821-to-v10.x

Conversation

@ckarande
Copy link
Contributor

@ckarande ckarande commented Jul 23, 2019

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
[`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

@addaleax addaleax Jul 23, 2019

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

@ckarande ckarande Jul 23, 2019

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

@ckarande ckarande Jul 24, 2019

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
* `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

@addaleax addaleax Jul 23, 2019

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
Loading

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

ckarande commented Jul 24, 2019

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

nodejs-github-bot commented Jul 25, 2019

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

nodejs-github-bot commented Jul 26, 2019

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

nodejs-github-bot commented Aug 2, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 18, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 18, 2019

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

BethGriggs commented Oct 7, 2019

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.

6 participants

@ckarande @nitinsurana @nodejs-github-bot @Trott @BethGriggs @addaleax