-
Notifications
You must be signed in to change notification settings - Fork 189
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
Renaming --identity to --authority in bench-tps #1435
Conversation
@KirillLykov @CriesofCarrots requesting your reviews. |
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.
In future, please don't tag random contributors asking for review
13939b0
to
46309b0
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.
Lgtm.
@KirillLykov , I'll let you confirm this satisfies what you wanted in your issue, and merge.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1435 +/- ##
========================================
Coverage 82.7% 82.7%
========================================
Files 872 872
Lines 370361 370389 +28
========================================
+ Hits 306478 306592 +114
+ Misses 63883 63797 -86 |
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.
Thanks for fixing this issue! Could you also add a small test to cli.rs
checking that identity
works?
Probably in the follow up PR, would be nice to rename identity into authority in the scripts that we use to run bench-tps.
is it okay if I add this to test
Also, can you say which scripts are used to run bench-tps? |
@kubanemil yes, something like that would fit. Regarding where it is used: in multinode-demo and some other shell scripts. Checkout particular places with |
I have checked out each shell script that uses |
You are right. In this case, less work. Please rebase with upstream and add the test if not already and I will approve and merge |
46deb14
to
ad9a01f
Compare
done |
Problem
The keypair in bench-tps is used to fund accounts in benchmarking, so it should be renamed to
--authority
Summary of Changes
Deprecated use of
--identity
. Substituted argument to--authority
in tests. Currently, both names are usable.Fixes solana-labs#30145