Fixes issues with --delete-data=on-conflict
#3730
Conversation
cloutiertyler
changed the title
There was a problem hiding this comment.
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.
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.
bfops
added
release-any
jdetter
added
release-1.10.0
and removed
release-any
… into tyler/fix-on-conflict
bfops
changed the title
--delete-data=on-conflict
Co-authored-by: Phoebe Goldman <phoebe@clockworklabs.io> Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
…ub.com:clockworklabs/SpacetimeDB into tyler/fix-on-conflict
jdetter
left a comment
jdetter
left a comment
There was a problem hiding this comment.
I haven't tested this but we at least test several different usages here. As Zeke pointed out, we need to add some more tests but we can do that later.
We also eventually want to change --features to just be a generic --build-args "..." where you can pass in arbitrary build options to build with (this would be useful for other langs as well). For now people can get around this by not using spacetime publish to build their wasm binary and instead build their wasm module manually and then use spacetime publish --bin-path ... .
I also made one small trivial change myself.
… into tyler/fix-on-conflict
|
|
||
| if clear_database != ClearMode::Always { | ||
| if clear_database == ClearMode::Always { | ||
| builder = confirm_and_clear(name_or_identity, force, builder)?; |
There was a problem hiding this comment.
note that this now only happens in the case where we've passed a name_or_identity, but that's required if we pass --clear-database.
| force: bool, | ||
| ) -> Result<reqwest::RequestBuilder, anyhow::Error> { | ||
| // The caller enforces this | ||
| assert!(clear_database != ClearMode::Always); |
There was a problem hiding this comment.
note: we had worrying behavior where one of the teams smoketests would fail if we were calling this function/endpoint unconditionally
There was a problem hiding this comment.
I created #3778 with the state of things that triggers that issue
There was a problem hiding this comment.
Created https://github.com/clockworklabs/SpacetimeDBPrivate/issues/2324 to track
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