`spacetime publish`: Add confirmation for `-c` #1038

bfops · 2024-04-04T20:27:56Z

Description of Changes

Making this change in response to a request from the BitCraft team.

spacetime publish -c now asks for confirmation, unless a new --force is passed as well.

API and ABI breaking changes

This will break any automation using spacetime publish -c.

I think that's an acceptable level of break? @jdetter ?

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

2

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

Testing

Basic publish still works fine, with no input:

  $ echo | spacetime publish -s local test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.15s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  Publishing module...
  Updated database with domain: test-module, address: 0d78dc56e0223a61218791b864064ab5


Publishing with `-c` (now) fails without confirmation:

  $ echo | spacetime publish -s local -c test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.16s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  This will DESTROY the current test-module module, and ALL corresponding data.
  Are you sure you want to proceed? (y/N) [deleting test-module] Aborting


...and it succeeds when given confirmation:

  $ echo 'y' | spacetime publish -s local -c test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.14s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  This will DESTROY the current test-module module, and ALL corresponding data.
  Are you sure you want to proceed? (y/N) [deleting test-module] Publishing module...
  Updated database with domain: test-module, address: 0d78dc56e0223a61218791b864064ab5


A "yes-like" answer is not good enough!

  $ echo 'yessir' | spacetime publish -s local -c test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.15s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  This will DESTROY the current test-module module, and ALL corresponding data.
  Are you sure you want to proceed? (y/N) [deleting test-module] Aborting

The clearly-named long flag works:

  $ echo | spacetime publish -s local --destroy-previous test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.15s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  This will DESTROY the current test-module module, and ALL corresponding data.
  Are you sure you want to proceed? (y/N) [deleting test-module] Aborting
  $ echo | spacetime publish -s local --destroy-previous --force test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.15s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  Skipping confirmation due to --force.
  Publishing module...
  Updated database with domain: test-module, address: 0d78dc56e0223a61218791b864064ab5

The deprecated long flag works like before:

  $ echo | spacetime publish -s local --clear-database test-module
  info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
  checking crate with spacetimedb's clippy configuration
      Checking spacetime-module v0.1.0 (/tmp/cramtests-v9blcwxj/publish-test.t/module-test)
      Finished release [optimized] target(s) in 0.14s
      Finished release [optimized] target(s) in 0.06s
  Optimising module with wasm-opt...
  Could not find wasm-opt to optimise the module.
  For best performance install wasm-opt from https://github.com/WebAssembly/binaryen/releases.
  Continuing with unoptimised module.
  Uploading to local => http://127.0.0.1:3000
  Skipping confirmation due to --force.
  Publishing module...
  Updated database with domain: test-module, address: 0d78dc56e0223a61218791b864064ab5

…r -c

jdetter

Let's not try to preserve the API here, just break it and we'll merge this as part of 0.9 next week.

jdetter · 2024-04-04T23:27:56Z

crates/cli/src/subcommands/publish.rs

+                .requires("name_or_address")
+                .conflicts_with("destroy_previous")
+                .action(SetTrue)
+                .help("DEPRECATED - Use --destroy-previous"),


Why did we add this?

I see this is a backwards compatible thing - let's just mark this as an API break and merge it for 0.9.

jdetter · 2024-04-04T23:30:22Z

crates/cli/src/subcommands/publish.rs

@@ -110,7 +125,8 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
    let name_or_address = args.get_one::<String>("name|address");
    let path_to_project = args.get_one::<PathBuf>("project_path").unwrap();
    let host_type = args.get_one::<String>("host_type").unwrap();
-    let clear_database = args.get_flag("clear_database");
+    let clear_database = args.get_flag("destroy_previous") || args.get_flag("deprecated_clear_database");


Let's just break the API here and not change this

jdetter · 2024-04-04T23:30:38Z

crates/cli/src/subcommands/publish.rs

@@ -163,6 +175,31 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
        database_host
    );

+    if clear_database {
+        // This is to preserve backwards-compatibility with the old --clear-database.
+        if force || args.get_flag("deprecated_clear_database") {


remove || args.get_flag("deprecated_clear_database")

jdetter · 2024-04-04T23:31:20Z

crates/cli/src/subcommands/publish.rs

-            // TODO(jdetter): Rename this to --delete-tables (clear doesn't really implies the tables are being dropped)
-            Arg::new("clear_database")
-                .long("clear-database")
+            Arg::new("destroy_previous")


let's keep this as clear-database - I realized that this was us trying to not break backwards compatibility but let's just break the API in this case.

I actually renamed this because of the TODO above - I'm fine to revert, but I do agree with it in principle!

cloutiertyler · 2024-04-16T01:05:15Z

Is there a reason we're renaming the long version, and not the short version? I don't think the inconsistency is worth it.

What is wrong with --clear-database?

jdetter · 2024-05-13T16:12:03Z

Is there a reason we're renaming the long version, and not the short version? I don't think the inconsistency is worth it.

What is wrong with --clear-database?

The feedback that you gave at the time was that --clear-database implies that you're just dropping all of the rows from the tables instead of deleting the tables completely and creating new ones.

jdetter · 2024-05-13T16:13:44Z

crates/cli/src/subcommands/publish.rs

-            Arg::new("clear_database")
-                .long("clear-database")
+            Arg::new("delete_tables")
+                .long("delete-tables")
                .short('c')


keeping the -c flag here kinda seems like its legacy. I believe we can't use -d because we're using that for the debug flag. Maybe we just keep --clear-database and drop the TODO, what do you think @bfops @cloutiertyler ?

Tyler already responded on this PR - let's just change this back to --clear-database and this is good to merge, thanks Zeke 👍

jdetter · 2024-05-13T16:14:55Z

crates/cli/src/subcommands/publish.rs

@@ -163,6 +167,30 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
        database_host
    );

+    if delete_tables {
+        if force {
+            eprintln!("Skipping confirmation due to --force.");


I would recommend just using println here instead of eprintln

jdetter · 2024-05-13T16:15:23Z

crates/cli/src/subcommands/publish.rs

+        }
+        query_params.push(("clear", "true"));
+    }
+
    eprintln!("Publishing module...");


Even though its not part of this PR I would recommend changing this to println as well

jdetter

Small request and then good to merge, thanks Zeke 👍

jdetter · 2024-05-13T16:17:02Z

crates/cli/src/subcommands/publish.rs

-            Arg::new("clear_database")
-                .long("clear-database")
+            Arg::new("delete_tables")
+                .long("delete-tables")
                .short('c')


Tyler already responded on this PR - let's just change this back to --clear-database and this is good to merge, thanks Zeke 👍

jdetter

LGTM, thanks Zeke 👍

bfops · 2024-05-14T15:41:16Z

smoketests/__init__.py

            "--project-path", self.project_path,
-            *(["-c"] if clear else []),


specifying -c without a domain is now a CLI error

added --force to avoid confirmation

bfops added 2 commits April 4, 2024 13:27

[bfops/confirm-publish-clear]: spacetime publish: add confirmation fo…

88b76b4

…r -c

[bfops/confirm-publish-clear]: review

1d4e0d2

bfops added bitcraft issue Active issue for the BitCraft team release-any To be landed in any release window labels Apr 4, 2024

bfops requested a review from jdetter April 4, 2024 20:32

bfops added 2 commits April 4, 2024 13:35

[bfops/confirm-publish-clear]: review

2596e77

[bfops/confirm-publish-clear]: review

ad29acc

bfops marked this pull request as ready for review April 4, 2024 21:39

[bfops/confirm-publish-clear]: review

a793ef2

jdetter reviewed Apr 4, 2024

View reviewed changes

bfops added 3 commits April 4, 2024 16:40

[bfops/confirm-publish-clear]: review

97c8d99

[bfops/confirm-publish-clear]: rename

dd64238

[bfops/confirm-publish-clear]: review

7d4a360

bfops requested a review from jdetter April 8, 2024 18:09

jdetter reviewed May 13, 2024

View reviewed changes

jdetter requested changes May 13, 2024

View reviewed changes

Zeke Foppa added 2 commits May 13, 2024 09:43

[bfops/confirm-publish-clear]: review

f969d29

[bfops/confirm-publish-clear]: review

d1b3f5d

bfops requested a review from jdetter May 13, 2024 16:49

jdetter approved these changes May 13, 2024

View reviewed changes

[bfops/confirm-publish-clear]: fix?

daecdb0

bfops enabled auto-merge May 13, 2024 17:47

Zeke Foppa added 2 commits May 14, 2024 08:36

[bfops/confirm-publish-clear]: fix smoketests

7f49eac

[bfops/confirm-publish-clear]: review

7f7e5e8

bfops commented May 14, 2024

View reviewed changes

[bfops/confirm-publish-clear]: fix rest of smoketests?

6db3cc6

bfops added this pull request to the merge queue May 14, 2024

Merged via the queue into master with commit 8ab55d2 May 14, 2024
7 checks passed

bfops deleted the bfops/confirm-publish-clear branch September 5, 2024 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`spacetime publish`: Add confirmation for `-c` #1038

`spacetime publish`: Add confirmation for `-c` #1038

bfops commented Apr 4, 2024 •

edited

Loading

jdetter left a comment

jdetter Apr 4, 2024

jdetter Apr 4, 2024

jdetter Apr 4, 2024

jdetter Apr 4, 2024

jdetter Apr 4, 2024

bfops Apr 4, 2024

cloutiertyler commented Apr 16, 2024

jdetter commented May 13, 2024

jdetter May 13, 2024

jdetter May 13, 2024

jdetter May 13, 2024

jdetter May 13, 2024

jdetter left a comment

jdetter May 13, 2024

jdetter left a comment

bfops May 14, 2024 •

edited

Loading

		"--project-path", self.project_path,
		*(["-c"] if clear else []),

spacetime publish: Add confirmation for -c #1038

spacetime publish: Add confirmation for -c #1038

Conversation

bfops commented Apr 4, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

jdetter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloutiertyler commented Apr 16, 2024

jdetter commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdetter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdetter left a comment

Choose a reason for hiding this comment

bfops May 14, 2024 • edited Loading

Choose a reason for hiding this comment

`spacetime publish`: Add confirmation for `-c` #1038

`spacetime publish`: Add confirmation for `-c` #1038

bfops commented Apr 4, 2024 •

edited

Loading

bfops May 14, 2024 •

edited

Loading