jackc / tern

The SQL Fan's Migrator
MIT License
925 stars 68 forks source link

Support fs.FS #29

Open thomasf opened 3 years ago

thomasf commented 3 years ago

Note sure about the NewFS function.

I haven't investigated how to test fs.FS stuff yet so I will add tests when I have had time to read how fs.FS works in more detail.

thomasf commented 3 years ago

It seems like there is a general issue with using packages from future go version even with a build tag guarding it https://github.com/golang/go/issues/40067

thomasf commented 3 years ago

Given the issue above it is maybe better to wait with built in fs.FS support until a new major version of tern which can require Go 1.16 and replaces MigratorFS with fs.FS entirely?

jackc commented 3 years ago

Given the issue above it is maybe better to wait with built in fs.FS support until a new major version of tern which can require Go 1.16 and replaces MigratorFS with fs.FS entirely?

There is a v2-experiment branch where I am trying a few new things. But I don't know if I will ever release it publicly. It's even more opinionated to how I personally do migrations. e.g. I removed "down" migrations as I now consider them an anti-pattern. It might be so specific to me that it is not useful to anyone else.

That said, I already replaced MigratorFS with http.FileSystem. Presumably it would be simple to go from http.FileSystem to fs.FS.

seh commented 3 years ago

It's even more opinionated to how I personally do migrations. e.g. I removed "down" migrations as I now consider them an anti-pattern.

Though not pertinent to this issue, I'd love to hear more of your opinions on how to write and use migrations better. If you've written about this elsewhere already, I appreciate any pointers you can share.

jackc commented 3 years ago

I haven't written this up anywhere but here's a short version.

A down migration doesn't make sense in production. If a migration adds a column the down migration would remove it. What about the data in that column? IMHO, it should always be rolled forward to explicitly deal with issues like that. Down migrations are only useful in development. But in development it is better to reset the database and remigrate.

The other thing is I have lately been more and more enamored with using server side functions and views. And with that I've come to the opinion that database migrations and database code should be treated separately. SQL code should be installed in its own schema and entirely replaced whenever there is a deploy. This entirely avoids issues with changing a view or function when other views or functions depend on it. (It also could let multiple versions temporarily coexist to allow zero downtime deploys.)

thomasf commented 3 years ago

I don't think many people wants to use down migrations in production, to me they are a last resort to roll back a failing upgrade that hopefully never has to be used. The few times I have used down migrations in production any added columns probably never have been populated to begin with.

For local development however I use them quite often to go between feature branches in some projects rolling back the database to master before switching branch.

jackc commented 3 years ago

For local development however I use them quite often to go between feature branches in some projects rolling back the database to master before switching branch.

In local development I find it faster / simpler to keep a development database premade as a template and and use createdb -T to create a fresh DB.

melekhine commented 2 years ago

Any news? Maybe we add that feature, who needs - will use.

thomasf commented 2 years ago

Even if we put this behind a build tag older Go versions go mod tidy breaks.

A fix has been backported to later versions of go 1.15.

My vote is to consider adding this somewhere after go 1.18 is released, by then people should be on a go version where this isn't an issue.

thomasf commented 2 years ago

I'll take this out of draft now since go 1.15 compatibility should not be something that is needed anymore because 1.15 has been out of support for a year now.

thomasf commented 2 years ago

I saw that the test was empty, putting back as a draft.