This is a simple change to rpcauth.py utility in order to output as json instead raw text.
This is beneficial because integrating json output is simpler with multiple different forms of automation and tooling
This is a simple change to rpcauth.py utility in order to output as json instead raw text.
This is beneficial because integrating json output is simpler with multiple different forms of automation and tooling
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, willcl-ark, tdb3, achow101 |
| Concept ACK | kristapsk |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Missing contrib: prefix in pull title? Also, the title should explain the change, both for the pull, as well as the commit title.
Concept ACK
Concept ACk
24 | @@ -24,6 +25,7 @@ def main(): 25 | parser = ArgumentParser(description='Create login credentials for a JSON-RPC user') 26 | parser.add_argument('username', help='the username for authentication') 27 | parser.add_argument('password', help='leave empty to generate a random password or specify "-" to prompt for password', nargs='?') 28 | + parser.add_argument("-json", help="output to json instead of plain-text", action='store_true')
The share/rpcauth/README.md file should also be updated to explain the argument and capability change in rpcauth.py.
nit:
Recommend making the argument be a single letter (e.g. -j for json) to follow the existing style with -h for help and prevent conflation with -j -s -o -n.
thank you. yes single "-" makes more sense.
ACK for 713ab485d7b0aa86ea5af35e44917ef15a5fb22a
Here's an example for the README.md:
diff --git a/share/rpcauth/README.md b/share/rpcauth/README.md
index 6b5e4446a5..1b3acb1dac 100644
--- a/share/rpcauth/README.md
+++ b/share/rpcauth/README.md
@@ -15,5 +15,5 @@ positional arguments:
optional arguments:
-h, --help show this help message and exit
- -j, -json output data in json format
+ -j, --json output data in json format
ACK for 713ab485d7b0aa86ea5af35e44917ef15a5fb22a. Cool feature. Inline comments provided.
Not sure it makes sense to do this. If you're parsing JSON, surely you can do the trivial things inside rpcauth yourself?
Not sure it makes sense to do this. If you're parsing JSON, surely you can do the trivial things inside rpcauth yourself?
Probably also true, my Bitcoin node setup script currently just does something like:
python3 "$SCRIPT_DIR/rpcauth.py" \
"$bitcoin_rpcuser" "$bitcoin_rpcpassword" | grep rpcauth >> /etc/bitcoin/bitcoin.conf
The default behavior is the same, so no change unless someone specifically wants json output.
For anyone just outputting "rpcauth=XXXXX" directly into bitcoin.conf, yes the grep solution is fine.
But if, in addition to that, you wanted to extract the value of rpcauth, then its more steps. (further, this assumes "=" will always be the delimiter)
By providing a machine-friendly output format that makes such deployments less fragile in the future.
Are you still working on this? #29433 (comment)
Apologies for delay - PR now has changed prefix / title. I also included update to README.md to document change.
36 | @@ -35,9 +37,13 @@ def main(): 37 | salt = generate_salt(16) 38 | password_hmac = password_to_hmac(salt, args.password) 39 | 40 | - print('String to be appended to bitcoin.conf:') 41 | - print(f'rpcauth={args.username}:{salt}${password_hmac}') 42 | - print(f'Your password:\n{args.password}') 43 | + if (args.json):
if args.json:
nit: This is not needed in python
Seems fine to add this.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits once they are ready for review.
Also, the contrib: prefix is still missing.
Thanks for the walk-thru.....I followed the steps so hopefully all is ok...
review ACK 4f746716c008216ff01da48ccfda559308b4232e
14 | @@ -15,4 +15,5 @@ positional arguments: 15 | 16 | optional arguments: 17 | -h, --help show this help message and exit 18 | + -j, -json output data in json format
I disagree with -json, should be --json, same as --help (not -help).
It makes sense to go with -j / --json for me as well.
Here's an example:
diff --git a/share/rpcauth/rpcauth.py b/share/rpcauth/rpcauth.py
index 5dd49c90b8..a40e981502 100755
--- a/share/rpcauth/rpcauth.py
+++ b/share/rpcauth/rpcauth.py
@@ -25,7 +25,7 @@ def main():
parser = ArgumentParser(description='Create login credentials for a JSON-RPC user')
parser.add_argument('username', help='the username for authentication')
parser.add_argument('password', help='leave empty to generate a random password or specify "-" to prompt for password', nargs='?')
- parser.add_argument("-json", help="output to json instead of plain-text", action='store_true')
+ parser.add_argument("-j", "--json", help="output to json instead of plain-text", action='store_true')
args = parser.parse_args()
if not args.password:
Are you still working on this?
Concept ACK.
Looks like these commits (4f746716c008216ff01da48ccfda559308b4232e, 1606590bbffa46f88fa6e4333960e82b919efd8a, and f2fb13bb1ac392abc4ccd656b025aa017b12060e) should be squashed to clean up and tell a concise story of update.
Seems to work well:
$ ./rpcauth.py
usage: rpcauth.py [-h] [-j] username [password]
rpcauth.py: error: the following arguments are required: username
$ ./rpcauth.py -h
usage: rpcauth.py [-h] [-j] username [password]
Create login credentials for a JSON-RPC user
positional arguments:
username the username for authentication
password leave empty to generate a random password or specify "-" to prompt for password
options:
-h, --help show this help message and exit
-j, --json output to json instead of plain-text
$ ./rpcauth.py tester1
String to be appended to bitcoin.conf:
rpcauth=tester1:5be9e5538a73ee796d73c16fe7267343$fb3462c51b9a943a5235eea45311100e637c47ef76e2ce00bd109d21940e231c
Your password:
j70a0dzR8gEZfdU3Su1c9jZi6k107iAMKiMdchHhrQE
$ ./rpcauth.py -j tester2
{"username": "tester2", "password": "NymKCrxvVxWvDd3XwPUaqgZp9Wr5cf3Kg6Flvss2b_U", "rpcauth": "tester2:7e1296561e4830d9987d38b32b2daf1d$d30aac9fd97726f9dcf6015508b01ec5ad9d1fd7055e9b61d619578b2bb871f5"}
$ ./rpcauth.py --json tester3
{"username": "tester3", "password": "kjhzSg-2jtizz4Krjj1KmQxrY0Vemf0jI3og2MTHFVs", "rpcauth": "tester3:f7a9248c1b11fec9efa1c681c42a9209$c68a4465920456f3cd99b322b58dbed50b9d4643cb13ce61228c9b8343a65bde"}
$ ./rpcauth.py --json tester4 throwawaypass
{"username": "tester4", "password": "throwawaypass", "rpcauth": "tester4:09156b6ab4a6ae5ab11508c4a6f5f68b$8804e2f73c22bac9532e40b3c6cdb2c61668bad9a4d7c67f4284bdcffb9bedc1"}
$ ./rpcauth.py tester5 anotherthrowaypass
String to be appended to bitcoin.conf:
rpcauth=tester5:563ad3d4b62f746c689f3853b0147ebe$7cf8ca75dd3f3623d79f2e5eeca6a69eefdd61b8d1546cba571ed6385ebc8dad
Your password:
anotherthrowaypass
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
-j/--json update and remove un-needed parens
Update README to reflect new -j/--json options
ACK 9adf949d2aa6d199b85295b18c08967395b5570a
tACK 9adf949d2aa6d199b85295b18c08967395b5570a
Makes sense to me to be able to output this in JSON for automated setups.
Thanks for squashing commits. ACK for 9adf949d2aa6d199b85295b18c08967395b5570a
ACK 9adf949d2aa6d199b85295b18c08967395b5570a