Skip to content

crypto: make DH error messages consistent - #31873

Closed
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-make-dh-error-messages-consistent
Closed

crypto: make DH error messages consistent#31873
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-make-dh-error-messages-consistent

Conversation

@tniessen

Copy link
Copy Markdown
Member

This custom error message was only added to make sure that #31445 was semver-minor.

Refs: #31178
Refs: #31445

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@tniessentniessen added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 19, 2020
@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 19, 2020
@jasnell

Copy link
Copy Markdown
Member

Since this is not changing the error code and only touches the error message, it does not need to be semver-major

@jasnelljasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 19, 2020
@tniessentniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 19, 2020
@tniessen

Copy link
Copy Markdown
MemberAuthor

@jasnell I think it needs to be, the error code was only added in #31445 (which was not semver-major since it didn't change the error name or message, just the code).

@jasnell

Copy link
Copy Markdown
Member

Ah, ok. That original pr should have been semver major

@tniessen

Copy link
Copy Markdown
MemberAuthor

Mhh really? I talked to another collaborator before creating the PR, and we came to the conclusion that adding a code (as opposed to changing an existing code) isn't breaking behavior, because it is unrealistic that existing code relies on the non-existence of the code property.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 9, 2020
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@tniessen

Copy link
Copy Markdown
MemberAuthor

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2297

Looks normal to me, then again, I find it very difficult to get meaningful information from the failures.

addaleax pushed a commit that referenced this pull request Mar 11, 2020
Refs: #31178
Refs: #31445
PR-URL: #31873
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax

Copy link
Copy Markdown
Member

Landed in 1b9a62c

@tniessentniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@tniessen@jasnell@nodejs-github-bot@addaleax@bnoordhuis@lpinca@cjihrig@devnexen@BridgeAR