jackc / tern

The SQL Fan's Migrator
MIT License
850 stars 66 forks source link

add dry run to tern #92

Open collinforsyth opened 4 months ago

collinforsyth commented 4 months ago

Adds a --dry-run feature to tern.

This is necessary in order to apply any templating logic necessary to output files, which can then be sent through any postgres linting tool if necessary.

As an example workflow, there's a pretty nice tool called squawk that outputs warnings on certain DDL changes, but arguably it could be any tool that requires valid Postgres DDL. These tools are becoming more and more popular leveraging the lib_pg_query so it's nice to be able to integrate with more static analysis.

❯ tern migrate -c dev.tern.conf -m ./migrations --dry-run | squawk
stdin:1:0: warning: prefer-bigint-over-int

   1 | -- file: 017_test.sql number: 17 - direction: up
   2 | CREATE TABLE test(foo INT);

  note: Hitting the max 32 bit integer is possible and may break your application.
  help: Use 64bit integer values instead to prevent hitting this limit.

find detailed examples and solutions for each rule at https://squawkhq.com/docs/rules

However, we use tern templated variables for some weirdness in dev/prod environments, so being able to run the migrations through tern (generating correct SQL), without executing them is a useful feature for static analysis.

Let me know if there's any feedback, off the top of my head some potentially contentious changes:

  1. Changing OnStart function specifically for dry run - this means that all tern output is essentially type-safe SQL. I think it might be reasonable to use that for all specific output.
  2. Still running tx.Begin() on --dry-run. I think the tests have it covered but am happy to also avoid creating any transaction if not necessary.

This is tested via all unit tests. Example of using on a local set up:

❯ tern migrate -c dev.tern.conf -m ./migrations --dry-run -d +1
-- file: 017_test.sql number: 17 - direction: up
CREATE TABLE test(foo INT);
jackc commented 4 months ago

I like the idea, but I don't think that *Migrate.MigrateTo is the correct location for the implementation. The branches in the main logic are unfortunate and the fmt.Printf should definitely not be there. What if an application that embedded tern wanted the dry run functionality without directly printing to stdout?

I would picture a new method on *Migrate that decides what a migrate operation needs to do. Maybe PlanMigrateTo() or PrepareMigrateTo. Not sure on the name. This method would factor out the logic for determining which migrations to run, whether it is up or down, and in which order the migrations run. The existing MigrateTo() would call this function. But the CLI could call directly PlanMigrateTo() for the --dry-run option.

This approach ensures that the dry run logic and the real migrate logic stay in sync, it allows applications that embed tern to use the dry run logic, and it avoids the unneeded transaction and reset all calls to the database when doing a dry run.

collinforsyth commented 4 months ago

I like the idea, but I don't think that *Migrate.MigrateTo is the correct location for the implementation. The branches in the main logic are unfortunate and the fmt.Printf should definitely not be there. What if an application that embedded tern wanted the dry run functionality without directly printing to stdout?

I would picture a new method on *Migrate that decides what a migrate operation needs to do. Maybe PlanMigrateTo() or PrepareMigrateTo. Not sure on the name. This method would factor out the logic for determining which migrations to run, whether it is up or down, and in which order the migrations run. The existing MigrateTo() would call this function. But the CLI could call directly PlanMigrateTo() for the --dry-run option.

This approach ensures that the dry run logic and the real migrate logic stay in sync, it allows applications that embed tern to use the dry run logic, and it avoids the unneeded transaction and reset all calls to the database when doing a dry run.

Agree on all accounts. Actually initially wanted to do that before opting for the simplest approach