-
Notifications
You must be signed in to change notification settings - Fork 645
Adds flag to publish to allow clearing on migration conflict. #3601
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: master
Are you sure you want to change the base?
Changes from all commits
835622f
7df8bed
d4446ef
339ca31
006cb79
33317ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ use spacetimedb_client_api_messages::name::{PrePublishResult, PrettyPrintStyle, | |
| use std::path::PathBuf; | ||
| use std::{env, fs}; | ||
|
|
||
| use crate::common_args::ClearMode; | ||
| use crate::config::Config; | ||
| use crate::util::{add_auth_header_opt, get_auth_header, AuthHeader, ResponseExt}; | ||
| use crate::util::{decode_identity, unauth_error_context, y_or_n}; | ||
|
|
@@ -16,12 +17,8 @@ pub fn cli() -> clap::Command { | |
| clap::Command::new("publish") | ||
| .about("Create and update a SpacetimeDB database") | ||
| .arg( | ||
| Arg::new("clear_database") | ||
| .long("delete-data") | ||
| .short('c') | ||
| .action(SetTrue) | ||
| common_args::clear_database() | ||
| .requires("name|identity") | ||
| .help("When publishing to an existing database identity, first DESTROY all data associated with the module"), | ||
| ) | ||
| .arg( | ||
| Arg::new("build_options") | ||
|
|
@@ -70,7 +67,7 @@ pub fn cli() -> clap::Command { | |
| Arg::new("break_clients") | ||
| .long("break-clients") | ||
| .action(SetTrue) | ||
| .help("Allow breaking changes when publishing to an existing database identity. This will break existing clients.") | ||
| .help("Allow breaking changes when publishing to an existing database identity. This will force publish even if it will break existing clients, but will NOT force publish if it would cause deletion of any data in the database. See --yes and --delete-data for details.") | ||
bfops marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
| .arg( | ||
| common_args::anonymous() | ||
|
|
@@ -97,15 +94,18 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E | |
| let server = args.get_one::<String>("server").map(|s| s.as_str()); | ||
| let name_or_identity = args.get_one::<String>("name|identity"); | ||
| let path_to_project = args.get_one::<PathBuf>("project_path").unwrap(); | ||
| let clear_database = args.get_flag("clear_database"); | ||
| let clear_database = args | ||
| .get_one::<ClearMode>("clear-database") | ||
| .copied() | ||
| .unwrap_or(ClearMode::Never); | ||
| let force = args.get_flag("force"); | ||
| let anon_identity = args.get_flag("anon_identity"); | ||
| let wasm_file = args.get_one::<PathBuf>("wasm_file"); | ||
| let js_file = args.get_one::<PathBuf>("js_file"); | ||
| let database_host = config.get_host_url(server)?; | ||
| let build_options = args.get_one::<String>("build_options").unwrap(); | ||
| let num_replicas = args.get_one::<u8>("num_replicas"); | ||
| let break_clients_flag = args.get_flag("break_clients"); | ||
| let force_break_clients = args.get_flag("break_clients"); | ||
|
|
||
| // If the user didn't specify an identity and we didn't specify an anonymous identity, then | ||
| // we want to use the default identity | ||
|
|
@@ -162,7 +162,7 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E | |
|
|
||
| let mut builder = client.put(format!("{database_host}/v1/database/{domain}")); | ||
|
|
||
| if !clear_database { | ||
| if !(matches!(clear_database, ClearMode::Always)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I don't know that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh can you do that? I think I didn't realize that. |
||
| builder = apply_pre_publish_if_needed( | ||
| builder, | ||
| &client, | ||
|
|
@@ -171,7 +171,9 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E | |
| host_type, | ||
| &program_bytes, | ||
| &auth_header, | ||
| break_clients_flag, | ||
| clear_database, | ||
| force_break_clients, | ||
| force, | ||
| ) | ||
| .await?; | ||
| }; | ||
|
|
@@ -181,7 +183,7 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E | |
| client.post(format!("{database_host}/v1/database")) | ||
| }; | ||
|
|
||
| if clear_database { | ||
| if matches!(clear_database, ClearMode::Always) || matches!(clear_database, ClearMode::OnConflict) { | ||
| // Note: `name_or_identity` should be set, because it is `required` in the CLI arg config. | ||
| println!( | ||
| "This will DESTROY the current {} module, and ALL corresponding data.", | ||
|
|
@@ -291,7 +293,9 @@ async fn apply_pre_publish_if_needed( | |
| host_type: &str, | ||
| program_bytes: &[u8], | ||
| auth_header: &AuthHeader, | ||
| break_clients_flag: bool, | ||
| clear_database: ClearMode, | ||
| force_break_clients: bool, | ||
| force: bool, | ||
| ) -> Result<reqwest::RequestBuilder, anyhow::Error> { | ||
| if let Some(pre) = call_pre_publish( | ||
| client, | ||
|
|
@@ -303,22 +307,37 @@ async fn apply_pre_publish_if_needed( | |
| ) | ||
| .await? | ||
| { | ||
| println!("{}", pre.migrate_plan); | ||
|
|
||
| if pre.break_clients | ||
| && !y_or_n( | ||
| break_clients_flag, | ||
| "The above changes will BREAK existing clients. Do you want to proceed?", | ||
| )? | ||
| { | ||
| println!("Aborting"); | ||
| // Early exit: return an error or a special signal. Here we bail out by returning Err. | ||
| anyhow::bail!("Publishing aborted by user"); | ||
| match pre { | ||
| PrePublishResult::ManualMigrate(manual) => { | ||
| if matches!(clear_database, ClearMode::OnConflict) { | ||
| println!("{}", manual.reason); | ||
| println!("Proceeding with database clear due to --delete-data=on-conflict."); | ||
| } | ||
| if matches!(clear_database, ClearMode::Never) { | ||
| println!("{}", manual.reason); | ||
| println!("Aborting publish due to required manual migration."); | ||
| anyhow::bail!("Publishing aborted by user"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this a correct bail message? Sure the user is ultimately responsible, but isn't it really just that they didn't choose to pass
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, yes it is. Good catch. Will fix. |
||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should we maybe assert that clear_database isn't
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good point. Will fix. |
||
| } | ||
| PrePublishResult::AutoMigrate(auto) => { | ||
| println!("{}", auto.migrate_plan); | ||
| // We only arrive here if you have not specified ClearMode::Always AND there was no | ||
| // conflict that required manual migration. | ||
| if auto.break_clients | ||
| && !y_or_n( | ||
| force_break_clients || force, | ||
| "The above changes will BREAK existing clients. Do you want to proceed?", | ||
| )? | ||
| { | ||
| println!("Aborting"); | ||
| // Early exit: return an error or a special signal. Here we bail out by returning Err. | ||
| anyhow::bail!("Publishing aborted by user"); | ||
| } | ||
| builder = builder | ||
| .query(&[("token", auto.token)]) | ||
| .query(&[("policy", "BreakClients")]); | ||
| } | ||
| } | ||
|
|
||
| builder = builder | ||
| .query(&[("token", pre.token)]) | ||
| .query(&[("policy", "BreakClients")]); | ||
| } | ||
|
|
||
| Ok(builder) | ||
|
|
@@ -333,7 +352,7 @@ async fn call_pre_publish( | |
| auth_header: &AuthHeader, | ||
| ) -> Result<Option<PrePublishResult>, anyhow::Error> { | ||
| let mut builder = client.post(format!("{database_host}/v1/database/{domain}/pre_publish")); | ||
| let style = pretty_print_style_from_env(); | ||
| let style: PrettyPrintStyle = pretty_print_style_from_env(); | ||
| builder = builder | ||
| .query(&[("pretty_print_style", style)]) | ||
| .query(&[("host_type", host_type)]); | ||
|
|
||
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.
I'm curious about the UX choice behind
require_equalsfor this arg but not othersUh oh!
There was an error while loading. Please reload this page.
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.
Because it's ambiguous and a little confusing otherwise:
spacetime publish --delete-data always foobar # Does this delete a database called "always" or does it always delete data?If we didn't do this you couldn't use the tool with a database named
alwayson-conflictornever. In practice that's probably not a problem, but sincerequires_equalscan always be relaxed, I thought we'd start by requiring it.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.