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.

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

_tls_wrap: Migrate the errors to internal/errors #17709 #17792

Closed
wants to merge 6 commits into from

Conversation

mannanali413
Copy link
Contributor

@mannanali413 mannanali413 commented Dec 20, 2017

Refs: #17709

Checklist
  • 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
Affected core subsystem(s)

The doc/api/errors.md file has been modified to include the ERR_TLS_RENEGOTIATION_DISABLED error .
Also, modified the files lib/_tls_wrap.js, lib/internal/errors.js, test/parallel/test-tls-disable-renegotiation.js so as to add the ERR_TLS_RENEGOTIATION_DISABLED error

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. labels Dec 20, 2017
@mannanali413 mannanali413 changed the title Migrate the errors to internal/errors.js lib/_tls_wrap.js #17709 _tls_wrap: Migrate the errors to internal/errors #17709 Dec 20, 2017
### ERR_TLS_RENEGOTIATION_DISABLED

This errors is triggered when attempts are made to renegotiate TLS on a socket
instance which has TLS disabled
Copy link
Member

Choose a reason for hiding this comment

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

The description is not quite accurate. Perhaps:

Triggered when an attempt is made to renegotiate the secure configuration on a TLS
socket after renegotiation has been disabled.

Copy link
Member

Choose a reason for hiding this comment

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

For consistency with other error descriptions, this should start with An attempt was made...

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 20, 2017
This migrates the old style error messages to use internal/errors.js

Fixes: nodejs#17709
This commit follows the previous commit. Tested Linting and added this commit

Fixes: nodejs#17709
This migrates the old style error messages to use internal/errors.js

Fixes: nodejs#17709
This commit follows the previous commit. Tested Linting and added this commit

Fixes: nodejs#17709
…TIATION_DISABLED

This commit paraphrases the error message corresponding to the ERR_TLS_RENEGOTIATION_DISABLED
key, so as to ensure consistency with the other error messages.

Fixes: nodejs#17709
@mannanali413
Copy link
Contributor Author

@jasnell I have paraphrased the message. Please let me know if this works :)

<a id="ERR_TLS_RENEGOTIATION_DISABLED"></a>
### ERR_TLS_RENEGOTIATION_DISABLED

An attempt was made to renegotiate TLS on a socket instance with TLS disabled.
Copy link
Member

Choose a reason for hiding this comment

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

tiny tiny nit: there is an extra space before disabled ;)

@addaleax
Copy link
Member

Remove the extra space in the message corresponding to the key ERR_TLS_RENEGOTIATION_DISABLED . 
Fixes: nodejs#17709
@maclover7
Copy link
Contributor

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2017
@joyeecheung
Copy link
Member

Landed in 79261f3, thanks for the contribution and welcome to Node.js!

joyeecheung pushed a commit that referenced this pull request Dec 23, 2017
This migrates the old style error in _tls_wrap.js to
the new style error ERR_TLS_RENEGOTIATION_DISABLED.

Refs: #17709

PR-URL: #17792
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants