Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support fs.FS #29

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Support fs.FS #29

wants to merge 1 commit into from

Conversation

thomasf
Copy link

@thomasf thomasf commented Feb 17, 2021

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
Copy link
Author

thomasf commented Feb 18, 2021

It seems like there is a general issue with using packages from future go version even with a build tag guarding it golang/go#40067

@thomasf
Copy link
Author

thomasf commented Feb 18, 2021

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
Copy link
Owner

jackc commented Feb 20, 2021

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
Copy link

seh commented Feb 20, 2021

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
Copy link
Owner

jackc commented Feb 20, 2021

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
Copy link
Author

thomasf commented Feb 20, 2021

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
Copy link
Owner

jackc commented Feb 20, 2021

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
Copy link

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

@thomasf
Copy link
Author

thomasf commented Sep 1, 2021

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.

@collinforsyth collinforsyth mentioned this pull request Nov 1, 2021
@thomasf thomasf mentioned this pull request Apr 13, 2022
@thomasf thomasf marked this pull request as ready for review April 13, 2022 13:16
@thomasf
Copy link
Author

thomasf commented Apr 13, 2022

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 thomasf marked this pull request as draft April 13, 2022 13:20
@thomasf
Copy link
Author

thomasf commented Apr 13, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants