-
Notifications
You must be signed in to change notification settings - Fork 317
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
feat: add script to verify parameter checksums in parameters.json #1251
Conversation
This commit adds a script to verify that a `.params` file (and the corresponding `.vk file`) is part of the `parameters.json` and has the correct checksums.
Will approve as soon as this is tested well. |
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.
Looks good, just some minor things.
scripts/verify-parameters-json.sh
Outdated
#!/bin/sh | ||
|
||
# This script verifies that a given `.params` file (and the corresponding | ||
# `.vk` file) is part of `parameters.json` as has the correct digest. |
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.
Minor typo 'as has the' = 'and has the'
scripts/verify-parameters-json.sh
Outdated
# utilities (`basename`, `head`, `grep`) as well as have `jq` and `b2sum` | ||
# installed. | ||
# | ||
# The input a `parameter.json` file and a `.params' file. |
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.
Wording is weird ('The input a')
scripts/verify-parameters-json.sh
Outdated
|
||
if [ "${#}" -ne 1 ]; then | ||
echo "Verify that a given .params file (and the corresponding .vk file)" | ||
echo "is part of parameters.json as has the correct digest." |
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.
Same as above
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.
Approved based on review of previous, though wording fixes per @cryptonemo would be worth making.
@vmx This tested well! |
@vmx I updated this, and also included a small fix that I did manually while verifying. I'd like to merge after CI is complete. |
@cryptonemo Thanks a lot for taking this on. |
This commit adds a script to verify that a
.params
file (and the corresponding.vk file
) is part of theparameters.json
and has the correct checksums.I extracted this script from this PR: #1249