-
Notifications
You must be signed in to change notification settings - Fork 283
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
fix(test-tooling): use of hardcoded password #3428
fix(test-tooling): use of hardcoded password #3428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you verify if the test cases requiring this piece of code clearly specifies test
as the password?
@jagpreetsinghsasan There are no test cases in the repo using this code/function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashnashahgrover Please fix the commit lint issues and document that it's a breaking change in the commit message (which then will get recognized by the change log and put in the release notes).
You can find the expected format for breaking changes in the conventional commit messages standard.
67e9390
to
8bdc830
Compare
@petermetz I have addressed these requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashnashahgrover The breaking change is that you made a previously optional parameter of the function mandatory. It doesn't refer to line breaks in text (although those can be important too because the commit linter will fail that as well).
Please refactor the PR description/commit message accordingly.
8bdc830
to
fc55bca
Compare
Have adjusted both the commit message and PR description accordingly. |
No it isn't updated @ashnashahgrover
|
fc55bca
to
a6778b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashnashahgrover I would recommend just copy pasting the commit message to the PR description so that it's a definite match character by character and see if that fixes the PR commit parity check. You can better see the differences if you open them up next to each other on two separate browser windows like this:
a6778b1
to
e187a84
Compare
e187a84
to
3c8404b
Compare
Currently this one is stuck because of a pending change request from @jagpreetsinghsasan @ashnashahgrover In case you didn't re-request review from @jagpreetsinghsasan , please do that. I'm not sure either way where did this one get stuck, but either which way let's get in moving! |
Primary Changes ---------------- 1. BREAKING CHANGE: "password" is now a mandatory parameter of the newEthPersonalAccount function defined in openethereum-test-ledger.ts. It was previously optional. 2. Updated line 236 in openethereum-test-ledger.ts so the default password argument to the newEthPersonalAccount function is not hardcoded. Fixes hyperledger-cacti#2766 Signed-off-by: ashnashahgrover <[email protected]>
3c8404b
to
e8c01fa
Compare
Commit to be reviewed
fix(test-tooling): use of hardcoded password
Fixes #2766
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.