Pretty print auto-migration plans #2065
Conversation
kazimuth
force-pushed
the
jgilles/pretty_print
branch
from
d66684b to
c0b04e1
Compare
kazimuth
force-pushed
the
jgilles/pretty_print
branch
from
c3de71d to
64ff276
Compare
kazimuth
changed the title
| @@ -189,6 +190,9 @@ pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error | |||
| } else { | |||
| println!("{} database with identity: {}", op, database_identity); | |||
| } | |||
| if let Some(update_summary) = update_summary { | |||
| println!("{}", update_summary); | |||
There was a problem hiding this comment.
TODO: I guess I should check if the terminal supports ANSI color codes here, since the summary may include those.
There was a problem hiding this comment.
Is this TODO meant for this PR, or should it be a comment/ticket?
There was a problem hiding this comment.
I'm not sure that this is the best UX. It seems like these update summaries can be pretty large, and I don't think that most users are interested in reviewing them. Could we put this behind a CLI flag --show-migration-summary or something like that?
There was a problem hiding this comment.
Just a reminder to myself. Also yeah I can put it behind a flag, sure.
TBH I'm not sure a flag is needed -- our tools are already pretty chatty and I don't think this adds an overwhelming amount of information. But probably a better long-term solution would be a history audit tool on the website; that would be a lot less ephemeral than console output. It's a larger project though.
I do think it's particularly important to print an explanation when automatic migrations fail -- but, come to think of it, this PR hasn't implemented that, so I suppose I should do it as well, either here or in another PR.
There was a problem hiding this comment.
Is that reminder meant to be implemented in this PR? (i.e. should I hold off on reviewing?)
There was a problem hiding this comment.
For this PR yeah, I'll mark as draft until it's ready again.
|
Please separate the bugfix from the new feature. These should be separate PRs. |
|
Sure, #2104 |
kazimuth
changed the title
|
Not my place but:
Is that a breaking a change? It sounds purely additive, which I would've called not-api-breaking? |
|
Converting to draft per https://github.com/clockworklabs/SpacetimeDB/pull/2065/files#r1910890326 |
Description of Changes
Implements https://github.com/orgs/clockworklabs/projects/22/views/16?filterQuery=migration&pane=issue&itemId=91111218.
While implementing this, I discovered auto-migration plans applied their steps in a nondeterministic order (I was iterating a hashmap and had forgotten to sort.)
This interacted in a particularly bad way with row-level-security policies:
This is now fixed by simply sorting the steps of an auto-migration in a prescribed way.
API and ABI breaking changes
This is a minor breaking change to the client API since it adds data to the result of an update call.
Expected complexity level and risk
0
Testing
Added a smoketest.
TODO: I want to add to the integration tests of the
schemacrate as well.