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

automagic sql db migrations #461

Merged
merged 4 commits into from
Nov 14, 2017
Merged

automagic sql db migrations #461

merged 4 commits into from
Nov 14, 2017

Conversation

rdallman
Copy link
Contributor

closes #57

migrations only run if the database is not brand new. brand new
databases will contain all the right fields when CREATE TABLE is called,
this is for readability mostly more than efficiency (do not want to have
to go through all of the database migrations to ascertain what columns a table
has). upon startup of a new database, the migrations will be analyzed and the
highest version set, so that future migrations will be run. this should also
avoid running through all the migrations, which could bork db's easily enough
(if the user just exits from impatience, say).

otherwise, all migrations that a db has not yet seen will be run against it
upon startup, this should be seamless to the user whether they had a db that
had 0 migrations run on it before or N. this means users will not have to
explicitly run any migrations on their dbs nor see any errors when we upgrade
the db (so long as things go well). if migrations do not go so well, users
will have to manually repair dbs (this is the intention of the migrate
library and it seems sane), this should be rare, and I'm unsure myself how
best to resolve not having gone through this myself, I would assume it will
require running down migrations and then manually updating the migration
field; in any case, docs once one of us has to go through this.

migrations are written to files and checked into version control, and then use
go-bindata to generate those files into go code and compiled in to be consumed
by the migrate library (so that we don't have to put migration files on any
servers) -- this is also in vcs. this seems to work ok. I don't like having to
use the separate go-bindata tool but it wasn't really hard to install and then
go generate takes care of the args. adding migrations should be relatively
rare anyway, but tried to make it pretty painless.

1 migration to add created_at to the route is done here as an example of how
to do migrations, as well as testing these things ;) -- created_at will be
0001-01-01T00:00:00.000Z for any existing routes after a user runs this
version. could spend the extra time adding 'today's date to any outstanding
records, but that's not really accurate, the main thing is nobody will have to
nuke their db with the migrations in place & we don't have any prod clusters
really to worry about. all future routes will correctly have created_at set,
and plan to add other timestamps but wanted to keep this patch as small as
possible so only did routes.created_at.

there are tests that a spankin new db will work as expected as well as a db
after running all down & up migrations works. the latter tests only run on mysql
and postgres, since sqlite3 does not like ALTER TABLE DROP COLUMN; up
migrations will need to be tested manually for sqlite3 only, but in theory if
they are simple and work on postgres and mysql, there is a good likelihood of
success; the new migration from this patch works on sqlite3 fine.

for now, we need to use github.com/rdallman/migrate to move forward, as
getting integrated into upstream is proving difficult due to
github.com/go-sql-driver/mysql being broken on master (yay dependencies).
Fortunately for us, we vendor a version of the mysql bindings that actually
works, thus, we are capable of using the mattes/migrate library with success
due to that. this also will require go1.9 to use the new database/sql.Conn
type, CI has been updated accordingly.

some doc fixes too from testing.. and of course updated all deps.

anyway, whew. this should let us add fields to the db without busting
everybody's dbs. open to feedback on better ways, but this was overall pretty
simple despite futzing with mysql.

@rdallman
Copy link
Contributor Author

just check out caf0b27 really to remove the noise.

@@ -68,7 +76,7 @@ var tables = [...]string{`CREATE TABLE IF NOT EXISTS routes (
}

const (
routeSelector = `SELECT app_name, path, image, format, memory, type, timeout, idle_timeout, headers, config FROM routes`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for audit purposes we need to add more than just created_at, what about updated_at, delete_at?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I agree -- read parent comment. this patch is about adding migrations, not adding timestamps. one was added so as to be an example so that others can do future ones. it's on my short list but don't want to bloat this patch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'm fine with adding them later, just wanted to clarify if that would happen "soon".

@@ -41,6 +48,7 @@ var tables = [...]string{`CREATE TABLE IF NOT EXISTS routes (
type varchar(16) NOT NULL,
headers text NOT NULL,
config text NOT NULL,
created_at text,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the route the only subject of an audit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see other response, patch not about this

Reed Allman added 2 commits October 26, 2017 11:44
closes #57

migrations only run if the database is not brand new. brand new
databases will contain all the right fields when CREATE TABLE is called,
this is for readability mostly more than efficiency (do not want to have
to go through all of the database migrations to ascertain what columns a table
has). upon startup of a new database, the migrations will be analyzed and the
highest version set, so that future migrations will be run. this should also
avoid running through all the migrations, which could bork db's easily enough
(if the user just exits from impatience, say).

otherwise, all migrations that a db has not yet seen will be run against it
upon startup, this should be seamless to the user whether they had a db that
had 0 migrations run on it before or N. this means users will not have to
explicitly run any migrations on their dbs nor see any errors when we upgrade
the db (so long as things go well). if migrations do not go so well, users
will have to manually repair dbs (this is the intention of the `migrate`
library and it seems sane), this should be rare, and I'm unsure myself how
best to resolve not having gone through this myself, I would assume it will
require running down migrations and then manually updating the migration
field; in any case, docs once one of us has to go through this.

migrations are written to files and checked into version control, and then use
go-bindata to generate those files into go code and compiled in to be consumed
by the migrate library (so that we don't have to put migration files on any
servers) -- this is also in vcs. this seems to work ok. I don't like having to
use the separate go-bindata tool but it wasn't really hard to install and then
go generate takes care of the args. adding migrations should be relatively
rare anyway, but tried to make it pretty painless.

1 migration to add created_at to the route is done here as an example of how
to do migrations, as well as testing these things ;) -- `created_at` will be
`0001-01-01T00:00:00.000Z` for any existing routes after a user runs this
version. could spend the extra time adding 'today's date to any outstanding
records, but that's not really accurate, the main thing is nobody will have to
nuke their db with the migrations in place & we don't have any prod clusters
really to worry about. all future routes will correctly have `created_at` set,
and plan to add other timestamps but wanted to keep this patch as small as
possible so only did routes.created_at.

there are tests that a spankin new db will work as expected as well as a db
after running all down & up migrations works. the latter tests only run on mysql
and postgres, since sqlite3 does not like ALTER TABLE DROP COLUMN; up
migrations will need to be tested manually for sqlite3 only, but in theory if
they are simple and work on postgres and mysql, there is a good likelihood of
success; the new migration from this patch works on sqlite3 fine.

for now, we need to use `github.com/rdallman/migrate` to move forward, as
getting integrated into upstream is proving difficult due to
`github.com/go-sql-driver/mysql` being broken on master (yay dependencies).
Fortunately for us, we vendor a version of the `mysql` bindings that actually
works, thus, we are capable of using the `mattes/migrate` library with success
due to that. this also will require go1.9 to use the new `database/sql.Conn`
type, CI has been updated accordingly.

some doc fixes too from testing.. and of course updated all deps.

anyway, whew. this should let us add fields to the db without busting
everybody's dbs. open to feedback on better ways, but this was overall pretty
simple despite futzing with mysql.
use rdallman/migrate until we resolve in mattes land
Copy link
Contributor

@hibooboo2 hibooboo2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems good. But it would be nice to have a read me some where of how he migration works. How to add / remove or change a field. So that in the future other devs can do this.

@rdallman
Copy link
Contributor Author

This all seems good. But it would be nice to have a read me some where of how he migration works. How to add / remove or change a field. So that in the future other devs can do this.

where should this go? our docs are a veritable cesspool atm. I would be okay with api/datastore/sql/migrations/README.me. In the same vein, I'm unsure of the contents to fill this with other than what can be seen in https://github.com/fnproject/fn/blob/migrations/api/datastore/sql/migrations/index.go -- which should become relatively obvious to inspect (the file) when a dev goes to actually try to add a field (vs. completely lost somewhere in the docs cesspool). as far as contents, is this enough: create a #_add_mything.sql file, run go generate, update base table & queries, that's it?

sql migrations seem like a relatively straightforward thing, I guess we simply need to define strict versioning of the migration files as well in index.go ? the migration(s) we have in our migrations package serve as pretty good examples anyway. wrt how it works, is this important or are you simply looking for constraints to things that won't work? again, it seems somewhat obvious that the migration package will apply in order each migration that the db it has is yet to run, this could be an additional line in index.go to clarify this.

let me know if this is inline with what you're thinking before I add, or what you are are actually thinking, please.

@hibooboo2
Copy link
Contributor

So a read me in the migrations package or a comment in the index.go would be nice. But as it stands it is not apparent to me How I would add a field and if anyone wants / needs to do this down the road it should not be cumbersome otherwise what's the point. Maybe a quick blurb in the index.go and a full example in a readme in the package for the migrations. I have dealt with migrations w/ a previous project and the process was not documented so you had to just know or talk with one of the lead devs every time. Just want to avoid that in this case.

@rdallman
Copy link
Contributor Author

@hibooboo2 please let me know if 47c8e3b makes it obvious how to add new hopefully-not-cumbersome migrations.

@rdallman
Copy link
Contributor Author

thanks, all. merging so we can all add fields now yey.

linking to mattes/migrate#299 -- would like to land so I can nuke my fork, damn mysql driver.

@rdallman rdallman merged commit 61b416a into master Nov 14, 2017
@rdallman rdallman deleted the migrations branch November 14, 2017 20:54
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.

Implement database schema migrations for painless upgrades
4 participants