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

Fix crypto::Hash update method's error output #25533

Closed
wants to merge 1 commit into from
Closed

Fix crypto::Hash update method's error output #25533

wants to merge 1 commit into from

Conversation

amitzur
Copy link
Contributor

@amitzur amitzur commented Jan 16, 2019

Fixes: #25487

This PR adds Buffer to the list of possible input types for Hash.update in the error for invalid input type. One test was added for verifying the error output is as expected.

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

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jan 16, 2019
@danbev
Copy link
Contributor

danbev commented Jan 21, 2019

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

@thefourtheye
Copy link
Contributor

I understand that this just includes a word in the error message (the functionality is already working as expected), should this be treated as a major change?

@addaleax
Copy link
Member

addaleax commented Feb 9, 2019

@thefourtheye I wouldn’t say so – we have an error code, so that should be okay.

Landed in e0a3d74, and thanks for the PR! 🎉

@addaleax addaleax closed this Feb 9, 2019
@addaleax addaleax reopened this Feb 9, 2019
addaleax pushed a commit that referenced this pull request Feb 9, 2019
@amitzur @addaleax
crypto: include 'Buffer' in error output of Hash.update method
e0a3d74 
Fixes: #25487

PR-URL: #25533
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Feb 10, 2019
@amitzur @targos
crypto: include 'Buffer' in error output of Hash.update method
8c9800c 
Fixes: #25487

PR-URL: #25533
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@thefourtheye
Copy link
Contributor

As the patch landed, this PR can be closed now. Thanks @amitzur for the PR 🙂

@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@sam-github sam-github sam-github approved these changes

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

Successfully merging this pull request may close these issues.
8 participants
@amitzur @danbev @thefourtheye @addaleax @sam-github @jasnell @benjamingr @nodejs-github-bot