Adds flag to publish to allow clearing on migration conflict. #3601
Conversation
| .short('c') | ||
| .num_args(0..=1) | ||
| .value_parser(value_parser!(ClearMode)) | ||
| .require_equals(true) |
There was a problem hiding this comment.
I'm curious about the UX choice behind require_equals for this arg but not others
| .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.") |
There was a problem hiding this comment.
nit: are we sure we want to encourage them to look at --yes?
| let mut builder = client.put(format!("{database_host}/v1/database/{domain}")); | ||
|
|
||
| if !clear_database { | ||
| if !(matches!(clear_database, ClearMode::Always)) { |
There was a problem hiding this comment.
nit: I don't know that matches! is more readable than == for simple enum values 🤷
| if matches!(clear_database, ClearMode::Never) { | ||
| println!("{}", manual.reason); | ||
| println!("Aborting publish due to required manual migration."); | ||
| anyhow::bail!("Publishing aborted by user"); |
There was a problem hiding this comment.
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 --clear-data? that feels like the user not doing something.
| println!("{}", manual.reason); | ||
| println!("Aborting publish due to required manual migration."); | ||
| anyhow::bail!("Publishing aborted by user"); | ||
| } |
There was a problem hiding this comment.
nit: should we maybe assert that clear_database isn't ClearMode::Always, or something similar? I feel like it takes me an extra pass or two reading this code to remember that the caller has chosen not to call this in that case.
bfops
left a comment
bfops
left a comment
There was a problem hiding this comment.
my code-owned changes LGTM, but left several small comments
Description of Changes
This PR modifies the
--clear-databaseflag onspacetime publishand adds the--clear-databaseflag onspacetime dev.In particular instead of
--clear-databasebeing a boolean, it is now a an enum:always-> corresponds to the old value oftruenever-> corresponds to the old value offalseon-conflict-> clears the database, but only if publishing would have required a manual migrationThis flag does NOT change any behavior about prompting users to confirm if they want to delete the data. Users will still be prompted to confirm UNLESS they pass the separate
--yesflag.spacetime devgets the same--clear-databaseflag. The default value ofneveris equivalent to the existing behavior.spacetime devcontinues to publish with--yesjust as before. This behavior is unchanged.API and ABI breaking changes
Adds the flags specified above. This is NOT a breaking change to the CLI. Passing
--clear-databaseis the equivalent of--clear-database=always.This IS technically a breaking change to the
pre_publishroute. As far as I'm aware this is only used by our CLI however.Expected complexity level and risk
2, Very small change, but if we get it wrong users could accidentally lose data. I would ask reviewers to think about ways that users might accidentally pass
--clear-database --yes.Testing