-
Notifications
You must be signed in to change notification settings - Fork 164
Add support for Cid-indexed data in forest-tool api generate-test-snapshot #5597
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
base: main
Are you sure you want to change the base?
Conversation
); | ||
let mut url = Url::parse(&base_url).ok()?; | ||
url.query_pairs_mut() | ||
.append_pair("cachebust", &Utc::now().timestamp().to_string()); |
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.
Let's document what is cachebust
.
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.
Done.
- run `cargo test --lib -- --test rpc_regression_tests --nocapture` | ||
- Manual Method | ||
|
||
1. Upload the test snapshots (`.zst` format is recommended) to the Digital Ocean space `forest-snapshots/rpc_test` |
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.
shall we mention to upload only .zst
format to the DO space for these test snapshots instead of recommending?
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.
Good idea, in practice, we always compress the files before uploading, and the script handles that automagically.
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👍🏻
scripts/tests/upload_rcpsnaps.sh
Outdated
BUCKET_URL="s3://${SPACE_NAME}/${DEST_PATH}" | ||
|
||
if zstd -f "$FILE_PATH" -o "$COMPRESSED_FILE"; then | ||
if s3cmd --quiet --no-progress put "${COMPRESSED_FILE}" "${BUCKET_URL}" --acl-public --mime-type="application/json" --add-header="Cache-Control: no-cache, no-store, must-revalidate"; then |
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.
s3cmd
is not a standard tool. It'd be good UX to have a check at the beginning of the script if it's in PATH and if not, nag the user.
scripts/tests/upload_rcpsnaps.sh
Outdated
DEST_PATH="${DEST_DIR}/${FILE_NAME}.zst" | ||
BUCKET_URL="s3://${SPACE_NAME}/${DEST_PATH}" | ||
|
||
if zstd -f "$FILE_PATH" -o "$COMPRESSED_FILE"; then |
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 general, can we simplify things via set -euo pipefail
? I think error handling would be better this way.
filecoin_chaingetevents_1745400136273439.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273611.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273670.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273739.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273790.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273834.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273881.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273923.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136273969.rpcsnap.json.zst | ||
filecoin_chaingetevents_1745400136274014.rpcsnap.json.zst |
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.
Do we need all of those snapshots? If yes, how are they different?
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.
We don't really need them all. I can do some trimming. ✂️
It'd be great to have a review from @hanabi1224 once he's back. |
Summary of changes
Changes introduced in this pull request:
forest-tool api generate-test-snapshot
rpc_regression_tests
rcpsnap
files to DOReference issue to close (if applicable)
Closes #5585
Other information and links
Change checklist