Automigration pretty print

[Shubham8287]

Description of Changes

Pretty print for Auto migration.

API and ABI breaking changes

NA

Expected complexity level and risk

1

Testing

• Added snapshot tests.

[gefjon]

This seems like a good opportunity to use insta for snapshot testing. I'd like to see three tests:

• Going from an empty schema to a populated schema (perhaps that of the module-test module, but perhaps just one that you write out yourself).
• Going from that populated schema to a different one that requires approval before migrating (I continue to think we should call this a "supervised migration" to distinguish it from a no-oversight automigration).
• Pretty-printing any schema with terminal colors disabled via NO_COLOR, to prove that we aren't going to fill peoples' logfiles with a bunch of horrible ANSI escape sequences in non-tty invocations. This test probably has to be marked #[serial] via the serial_test crate and run in a test target (i.e. file under crates/foo/tests/) that only has explicitly marked tests due to set_var being unsafe in multithreaded contexts.

[Shubham8287]

Makes sense to add snapshot tests. I will do that.

This test probably has to be marked #[serial] via the serial_test crate and run in a test target (i.e. file under crates/foo/tests/) that only has explicitly marked tests due to set_var being unsafe in multithreaded contexts.

umm, I think using set_var is only useful if we are writting this test from spacetimedb-cli interface. I imagine flow to be like:

1. spacetime-cli checks for NO_COLOR env variable.
2. Accordingly call pre_publish api with param to use PlainFormatter instead of AnsiFormatter.

So test for disabling ANSI escapes based on the passed argument should be enough?

[gefjon]

umm, I think using set_var is only useful if we are writting this test from spacetimedb-cli interface. I imagine flow to be like:

1. `spacetime-cli` checks for `NO_COLOR` env variable.

2. Accordingly call `pre_publish` api with param to use `PlainFormatter` instead of `AnsiFormatter`.

So test for disabling ANSI escapes based on the passed argument should be enough?

Sure, if you set it up that way, that'd work fine. It would be unsafe if you relied on the colored crate to check NO_COLOR, and used set_env within the test to do the assertion. Does PlainFormatter already exist, or are you planning to add it separately?

[Shubham8287]

Does PlainFormatter already exist, or are you planning to add it separately?

I stole one from James draft PR - https://github.com/clockworklabs/SpacetimeDB/pull/2065/files#diff-8150155bc61f819c6f1e3b414e728839462b5421657e2393304e4d050fd69cbdR18.

Regex wrapper over ANSI formmater. It gets the job done.

[gefjon]

Tests seem to be failing in CI. I wonder if our CI runs with NO_COLOR set, or something, and so colored is not printing the colors in that environment.

I can't say I'm super jazzed about writing the colors and then stripping them out again with regexes, but I also don't really care enough to complain.

[gefjon]

I don't think this is a correct use of the colored crate. Honestly, I don't know of colored is correct for what we're trying to do here. If we do use it, though, I don't think this is the right way. set_override, perhaps obviously, sets a global flag, and so setting it and then never unsetting it the way you do here will break any other uses of the crate in the same process which rely on the environment-checking behavior. We already have a dependency on and uses of colored prior to this PR, so that doesn't seem like a great idea. It would probably be fine to just make sure we unset that flag after finishing formatting, since our CLI is effectively single-threaded, but that still makes me pretty unhappy. I think I would prefer either:

• Having only a single formatter which uses colored's machinery for inspecting the environment and does not override, and then having #[serial] tests which manipulate the environment or use the override to test the output.
• Using a different library which is focused on expressiveness rather than on being "The most simple way to add colors in your terminal," like termcolor, which allows locally overwriting the color choice.

[Shubham8287]

set_override, perhaps obviously, sets a global flag, and so setting it and then never unsetting it the way you do here will break any other uses of the crate in the same process which rely on the environment-checking behavior.

I didn't quite understand why the environment matters here. Isn't the schema crate internal and intended to be used on the server side only?

Here, we're basically specifying that the code colors the plan as requested, rather than depending on the local environment — which seems right to me. Local env setting should matter where the CLI running not at the server code. NO?

We already have a dependency on and uses of colored prior to this PR, so that doesn't seem like a great idea.

I can't think of any potential use case that this would break. If you can provide one, I may get convinced.

[gefjon]

set_override, perhaps obviously, sets a global flag, and so setting it and then never unsetting it the way you do here will break any other uses of the crate in the same process which rely on the environment-checking behavior.

I didn't quite understand why the environment matters here. Isn't the schema crate internal and intended to be used on the server side only?

Here, we're basically specifying that the code colors the plan as requested, rather than depending on the local environment — which seems right to me. Local env setting should matter where the CLI running not at the server code. NO?

Is your intention that we pretty-print and colorize the schema on the server, and then send the text, including ANSI escapes, over HTTP? That seems wrong to me; I had assumed we would send the plan as JSON, and then have the CLI pretty-print it as desired. Note that ANSI coloring is not the only standard for coloring in terminals - some Windows terminals use something else, and alternative user agents would have entirely other mechanisms for colorizing output (e.g. a web client like our website or this unofficial one would use HTTP and CSS; a native GUI system would just draw colored text or pixels).

We already have a dependency on and uses of colored prior to this PR, so that doesn't seem like a great idea.

I can't think of any potential use case that this would break. If you can provide one, I may get convinced.

Assuming we do go ahead with pretty-printing on the server (which I object to, but let's set that aside for the time being), we later decide that some other thing should be colorized, e.g. something in SpacetimeDB's log output. This printing should actually respect NO_COLOR, as it's going to be printed directly to stdio, which may be going to a user's TTY or to some CI runner or god knows what else. We already have a dependency on colored, so we just go ahead and use that. This behaves correctly and respects NO_COLOR at startup, but then as soon as a user goes to pre-publish a new version, the logs start ignoring NO_COLOR and printing ANSI escape sequences into Elasticsearch.

[Shubham8287]

Yes, I intended to do it at the Server and send over HTTP.
And I know its not great but It has been done this way because MigratePlan type does not look trivial to serialize, its deeply nested with lifetimes.

I beleive James took similar #2065 approach in previous attempt due to same reason.

I have also mentioned about this subtly here -#2962 (comment), Though, I should have mentioned more clearly (I mistakenly assumed it a common knowledge and agreed-upon approach).

I agree, sending JSON and prettify-ing at client is Correct and should have done, its just an extra effort.

[gefjon]

Yes, I intended to do it at the Server and send over HTTP. And I know its not great but It has been done this way because MigratePlan type does not look trivial to serialize, its deeply nested with lifetimes.

I beleive James took similar #2065 approach in previous attempt due to same reason.

I have also mentioned about this subtly here -#2962 (comment), Though, I should have mentioned more clearly (I mistakenly assumed it a common knowledge and agreed-upon approach).

I agree, sending JSON and prettify-ing at client is Correct and should have done, its just an extra effort.

If that is indeed the plan, then I feel strongly that using colored in this way is a ticking time bomb waiting to turn into an extremely obnoxious and hard-to-debug bug. This is clearly not the intended use case for that library and their API does not support what we're trying to do here.

I am comfortable deferring sending a structured format like JSON over the wire as future work and using this design as a stop-gap solution, but please make a ticket so we can track this. Note that we do not need to (and indeed should not) serialize the actual auto-migration plans directly into JSON (that would expose them and require that we stabilize them, which we don't want to do), instead we would serialize to a semi-structured intermediate format which clients can then pretty-print as appropriate.

[Shubham8287]

Checking out termcolor, ColorChoice looks great. It will also let us implement PlainFormatter without regex hack.

[Shubham8287]

@gefjon - #3184