-
Notifications
You must be signed in to change notification settings - Fork 649
Fixes issues with --delete-data=on-conflict
#3730
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
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.
From a black box perspective, I would like you to test the following cases, with each value of --delete-data:
- no migration required
- auto-migrateable (with the yes-break flag true/false)
- manual migration required
Edit: I would like to consider making these into automated tests!
Given the original issue, please also spot-test spacetime dev.
|
Shall we do a hotfix release with this? |
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.
Pull request overview
This PR fixes issues with the --delete-data=on-conflict flag in the SpacetimeDB CLI's publish command. The main problem was that the data deletion prompt and logic were being executed at the wrong time in the publish flow, and the flag wasn't being properly respected when auto-migration was possible.
Key changes:
- Fixed the logic for
--delete-data=on-conflictto only delete data when a manual migration is required, not when auto-migration is available - Moved data deletion prompts and logic into the
apply_pre_publish_if_neededfunction where migration decisions are made - Added support for
--featuresflag to enable building Rust modules with different feature sets for testing - Added comprehensive integration tests to verify all migration scenarios
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cli/src/subcommands/publish.rs | Refactored data deletion logic to occur within migration decision flow, fixed handling of --delete-data=on-conflict |
| crates/cli/src/subcommands/build.rs | Added --features flag support for Rust builds |
| crates/cli/src/tasks/rust.rs | Updated build function to accept and pass features to cargo |
| crates/cli/src/tasks/mod.rs | Added features parameter and validation for non-Rust modules |
| crates/cli/tests/publish.rs | Added comprehensive integration tests for all publish/migration scenarios |
| crates/cli/tests/util.rs | Added utility for spawning SpacetimeDB instances in tests |
| crates/cli/tests/server.rs | Added basic server connectivity test |
| modules/module-test/src/lib.rs | Added conditional compilation features for testing migrations |
| modules/module-test/Cargo.toml | Added feature flags for migration testing |
| crates/testing/src/modules.rs | Updated to pass new features parameter |
| crates/cli/src/subcommands/dev.rs | Updated to use new build API with features parameter |
| crates/codegen/tests/snapshots/*.snap | Updated snapshots to include generated code for new test table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… into tyler/fix-on-conflict
Signed-off-by: Zeke Foppa <[email protected]>
jdetter
left a 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.
Just one question but no objections from me on this, thanks for cleaning this one up Zeke 🙏
I have not tested this myself but this was in last week's release and we haven't seen any complaints about this issue since the last release.
Description of Changes
Fixes #3729
I genuinely don't know what came over me.
API and ABI breaking changes
None
Expected complexity level and risk
1.5 very straightforward but not strictly trivial
Testing
Adds automated integration tests (written in Rust and run with
cargo test, although note this comment from @matklad about integration tests for the future https://internals.rust-lang.org/t/running-test-crates-in-parallel/15639/2):--delete-data--delete-data=on-conflict--delete-data=on-conflictis specified