karenc / db-migrator

GNU Affero General Public License v3.0
4 stars 5 forks source link

Add repeat conditional migrations #39

Closed karenc closed 7 years ago

karenc commented 7 years ago

If the migration file has should_run defined, it is considered a repeat migration. should_run is called every time migrate is run, and if it returns true, up is run.

Close Connexions/db-migrator#1

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.9%) to 97.056% when pulling 0e30aeee9bd4f078e3b647c8269a60c8fc169931 on repeat-migrations into 5091c38549aac7cfadf668917aac0a367b40ba35 on master.

karenc commented 7 years ago

@reedstrm Can you have a look at this? We can update Connexions/db-migrator when the changes are merged.

karenc commented 7 years ago

Dynamic Migrations

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.7%) to 96.851% when pulling 7cfa9fb0ca271b61974bd279458313e88dbf92f4 on repeat-migrations into 5091c38549aac7cfadf668917aac0a367b40ba35 on master.

reedstrm commented 7 years ago

:+1:

reedstrm commented 7 years ago

It occurs to me that eventually, we'll have a huge number of "repeatable" migrations, which are marked "applied" (with the most recent application timestamp, I presume?) But that must be tested every time any migration happens, even 10 years later when they no longer apply. Even the test may no longer apply. I guess there's nothing that says we can't "cosolidate" migrations by deleting them, meaning we no longer support migrating from the state "just in front of" that migration. I'd like some way to tag a migration as "no need to use this one ever again on this database" - a terminal "no more repeats" .For the cases where the DBA/devops can say "the code that generates that bad data no longer runs, we do not need to migrate anymore"

Since the existing table is "schema_migrations" does it make sense to instead track these as "data_migrations". The "should_run" function could be described as "run tests against existing data to determine if there is work for this data migration to do". That's what I intend to write, BTW - @deferred + repeatable with a should_run that checks for markers of what data has been converted, and an up that converts a subset of data, and marks it (by making a backup?), so down can put it back. :-)

reedstrm commented 7 years ago

If presence of should_run makes a migration repeatable, how about having the absence of a down mark a migration as irreversible, and make it only run if --forced ?

karenc commented 7 years ago

dbmigrator migrate already has an option --version for running migrations up to a certain version, we can alias it to --to-version and add a --from-version so we can exclude some old repeatable migrations.

karenc commented 7 years ago

I think if we add irreversible migrations that are only run if --forced is used, we're adding complexity to the order of the migrations. The migrations are meant to be ordered by the timestamp. We are probably not going to test running the migrations in different order, it might fail if we have a large number of migrations that need to be run.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.7%) to 96.848% when pulling b4839b805ff59cc803899174fefd55d50031fc51 on repeat-migrations into 4a82ebf440ce52b7fca36a6dc3b9211ef33445ad on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.7%) to 96.841% when pulling 00e4473201dc3873c779b1d7de0a3d9d736fb78e on repeat-migrations into 4a82ebf440ce52b7fca36a6dc3b9211ef33445ad on master.

karenc commented 7 years ago

Now that I think about it, if you comment out should_run, the repeatable migration won't run anymore. Will that be enough?

reedstrm commented 7 years ago

Turns out that, in order to be able to do a "down" for the data migration, I needed to store my own state anyway, so no need for extra table stuff. This seems to be working asis for me as well. I haven't ran it a bunch yet on a larger test data set, which is my actual use-case: I want to have an 'up' that only does part of the work, so is worth calling repeatedly. I'll assume that the date applied will update to reflect the last_date applied. THe only problem is with that isapplied is no longer enough info in list to know if a migrate will do anything, so we need an indication of repeatability. OH, ow about an asterisk? True* means will try to run again (could even have False*,but it's not needed) And it looks like a regex or glob, meaning "many" :-)

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.8%) to 96.988% when pulling 68e4c2f1a65213c7908519102a8b036cf5926b2d on repeat-migrations into 4a82ebf440ce52b7fca36a6dc3b9211ef33445ad on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.8%) to 97.009% when pulling 0dae39884092d26c21d98d84bef26da380866699 on repeat-migrations into 4a82ebf440ce52b7fca36a6dc3b9211ef33445ad on master.