-
Notifications
You must be signed in to change notification settings - Fork 182
fix(links): update the script to use latest calibnet snapshot #6117
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
Conversation
WalkthroughChange Calibnet snapshot source to the v1 archive URL and update the Calibnet DB migration test: parameterize initial Forest version (set to 0.30.0), adjust Docker mount path, add migration orchestration flags and post-migration checks; no public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestScript as Test Script
participant DockerOld as Old Forest (container)
participant FS as Host FS
participant CurrentForest as Current Forest (container)
participant API as Local API
Note over TestScript: Setup old DB using FOREST_INIT_VERSION
TestScript->>DockerOld: run old forest with mounted DB (/root/.local/share/forest)
DockerOld-->>FS: writes old DB directory
TestScript->>CurrentForest: run current forest with config.toml pointing to old DB\n(flags: --auto-download-snapshot, --halt-after-import)
CurrentForest->>FS: import/upgrade DB (migration)
CurrentForest->>API: start API for post-migration checks
API-->>TestScript: migration status / validate data
TestScript->>FS: assert old-version directory removed
Note over TestScript: End of migration validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli_shared/snapshot.rs (1)
135-138: Optional: append trailing slash tolatest-v1/calibnetand align all references
Bothhttps://…/latest-v1/calibnetand with a trailing slash return 200, but adding/avoids a potential redirect and matches themainnet/URL.src/cli_shared/snapshot.rs -const FOREST_CALIBNET_COMPRESSED: &str = - "https://forest-archive.chainsafe.dev/latest-v1/calibnet"; +const FOREST_CALIBNET_COMPRESSED: &str = + "https://forest-archive.chainsafe.dev/latest-v1/calibnet/";Also update the URL in
scripts/tests/harness.sh(line 32) and the README entry under “Forest snapshot links” to include the trailing slash.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/tests/harness.sh(1 hunks)src/cli_shared/snapshot.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T14:17:09.129Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5978
File: .github/workflows/unit-tests.yml:0-0
Timestamp: 2025-08-25T14:17:09.129Z
Learning: hanabi1224's download_file_with_cache function in src/utils/net/download_file.rs preserves URL path structure in local cache directories by using cache_dir.join(url.path().strip_prefix('/').unwrap_or_else(|| url.path())), so snapshots from https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/ are cached locally at ~/.cache/forest/test/rpc-snapshots/rpc_test/ (including the rpc_test subdirectory from the URL path).
Applied to files:
src/cli_shared/snapshot.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (1)
scripts/tests/harness.sh (1)
32-32: Calibnet v1 snapshot URL switch looks good.This aligns the harness with the new archive endpoint and matches the F3 merge flow.
Optionally verify the endpoint resolves to a .forest.car.zst with sane headers and redirects:
|
@LesnyRumcajs I guess for |
I think we can actually be more pragmatic. We don't expect anyone to run Forest pre |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/tests/calibnet_db_migration.sh (2)
4-5: Align migration comment with bumped base version.The header comment still states we migrate from 0.19.2, but the script now seeds with 0.30.0. Please update the wording so future readers aren’t misled.
-# This script tests the migration(s) from Forest 0.19.2 to the current version. +# This script tests the migration(s) from Forest 0.30.0 to the current version.
16-23: Refresh inline comment to reflect FOREST_INIT_VERSION.Same issue here: the narrative still references 0.19.2 while we now launch 0.30.0. Tweaking the comment keeps the instructions accurate.
-# Run Forest 0.19.2 with mounted db so that we can re-use it later. +# Run Forest ${FOREST_INIT_VERSION} with a mounted db so that we can re-use it later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tests/calibnet_db_migration.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/tests/calibnet_db_migration.sh (1)
14-19: Refresh baseline version references in comments.We now seed the DB with v0.30.0, but the surrounding comments still talk about 0.19.2. Keeping them in sync avoids future confusion while debugging CI runs.
-# This script tests the migration(s) from Forest 0.19.2 to the current version. +# This script tests the migration(s) from Forest 0.30.0 to the current version. … -# Run Forest 0.19.2 with mounted db so that we can re-use it later. +# Run Forest 0.30.0 with mounted db so that we can re-use it later. … -# Assert there is no "0.19.2" directory in the database directory. This and a successful sync indicate that the database was successfully migrated. +# Assert there is no "0.30.0" directory in the database directory. This and a successful sync indicate that the database was successfully migrated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tests/calibnet_db_migration.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: tests
- GitHub Check: tests-release
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/calibnet_db_migration.sh (1)
67-71: Version parsing will fail on release builds without ‘+’
sed -E 's/.* (.*)\+.*/\1/'only works whenforest --versioncontains a+…suffix. On plain releases it yields the full string (e.g., “forest 0.31.0”) and breaks the path check.-# Get current Forest version -CURRENT_VERSION=$(forest --version | sed -E 's/.* (.*)\+.*/\1/') +# Get current Forest version (strip any +metadata suffix) +CURRENT_VERSION=$(forest --version | awk '{print $NF}' | cut -d+ -f1)
🧹 Nitpick comments (4)
scripts/tests/calibnet_db_migration.sh (4)
14-14: Allow overriding FOREST_INIT_VERSION via envHardcoding makes the job less flexible. Default to 0.30.0 but let CI override.
-FOREST_INIT_VERSION="0.30.0" +FOREST_INIT_VERSION="${FOREST_INIT_VERSION:-0.30.0}"
16-23: Volume path may differ across images; mount both root and non-root locationsOlder images may run as user “forest” with HOME=/home/forest while newer run as root. Dual‑mount avoids path drift and removes ambiguity.
docker run --init --rm --name forest-${FOREST_INIT_VERSION} \ - --volume "${DATA_DIR}":/root/.local/share/forest \ + --volume "${DATA_DIR}":/root/.local/share/forest \ + --volume "${DATA_DIR}":/home/forest/.local/share/forest \ ghcr.io/chainsafe/forest:v${FOREST_INIT_VERSION} \ --chain calibnet \ --encrypt-keystore false \ --auto-download-snapshot \ --halt-after-importOptional: add
--pull=alwaysto avoid stale tag usage.
33-35: Don’t assume sudo availabilityGH-hosted runners have sudo, but self-hosted may not. Fallback to plain chown when sudo is absent.
-if [ ! -w "${DATA_DIR}/calibnet/${FOREST_INIT_VERSION}" ]; then - sudo chown -R "$(id -u):$(id -g)" "${DATA_DIR}/" -fi +if [ ! -w "${DATA_DIR}/calibnet/${FOREST_INIT_VERSION}" ]; then + if command -v sudo >/dev/null 2>&1; then + sudo chown -R "$(id -u):$(id -g)" "${DATA_DIR}/" + else + chown -R "$(id -u):$(id -g)" "${DATA_DIR}/" + fi +fi
45-59: Ensure background forest is cleaned upAdd a trap to kill background processes to avoid leaks on failure.
/usr/bin/time -v forest --chain calibnet --log-dir "$LOG_DIRECTORY" --save-token ./admin_token --config "${CONFIG_FILE}" & +trap 'kill $(jobs -p) 2>/dev/null || true' EXIT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tests/calibnet_db_migration.sh(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (1)
scripts/tests/calibnet_db_migration.sh (1)
61-65: LGTM: assert old-version dir removalThis check is a good guard to confirm migration completed and cleanup happened.
If migrations intentionally retain a tombstone/marker in older versions, consider checking for a non-empty directory instead:
[ -d dir ] && [ -z "$(ls -A dir)" ].
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Part of the #6115
Other information and links
Change checklist
Summary by CodeRabbit
Bug Fixes
Tests
Chores