-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
First pass at migrations #64
Conversation
Review? @mfpiccolo @samphippen @mcasper @mikeastock |
use std::path::{Path, PathBuf}; | ||
|
||
pub trait Migration { | ||
fn version(&self) -> i64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a string? Feels like the property you want here is sortable, not "only a number". For example: leading zeroes might be useful, or users might want to have hexadecimal or similar. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one concern with that. If we assume it's always a number, we have a clearly defined way to determine what is and isn't a migration in this directory (for example, we shouldn't error when we see .gitkeep
). We'd need to make sure we have a strict definition, as I do want to error in the case where something looks an awful lot like a migration, but is off somehow (down.sql
being missing for example). Is ignoring files with a leading .
enough here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes on ignoring .
Having a little bit of trouble getting set up on my test project. I got the migrations to run from build.rs, but I am now getting:
when using Or:
when using |
@mfpiccolo Do you have a primary key other than |
Dumb mistake on my part! Everything is working as expected. Nice work @sgrif |
let entry = t!(e); | ||
let file_name = entry.file_name(); | ||
&file_name == "up.sql" || &file_name == "down.sql" | ||
}) && t!(path.read_dir()).count() == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we potentially make this >= 2
? In case some projects or companies want to be able to have other files in the dir, like example_output.txt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this falls under the same umbrella as https://github.com/sgrif/diesel/pull/64/files#r48106745 -- I think it'll be fine to ignore files starting with .
, but beyond that we want to make sure it contains exactly what's expected.
53b389b
to
98e28a9
Compare
This is the first (small) piece of migration support, finding the directory containing migrations. Searches the current directory and its parents until it finds the directory containing Cargo.toml.
This still has a lot of rough edges, and isn't a fully finished feature, but it's pretty much the smallest step to something that I can test that passes. This adds the ability to run pending migrations (presumably from a build script). Migrations should be in the directory /migrations, where each migration is a timestamped sub-directory containing the files `up.sql` and `down.sql`. The migrations will be run, and an internal table will keep track of which migrations have or haven't been run. A lot of this code needs to be refactored.
Interestingly enough, the easiest way to accomplish this is to just use our own DSLs. We can't use codegen here, as that would create a circular dependency, but we just need to implement `Insertable` by hand. There were a few strange quirks that came as a result of this. I had to move `macros` up to the top so it could be used in `migrations`, which meant it could not do `use $crate::*;`. Additionally, we once again run into the bug where we can only use traits in public modules in our own crate.
We need to be sure that the migration running successfully and the insertion into `__diesel_schema_migrations` are an atomic operation. I wish I could write the generic `impl<T: From<Error>> From<TransactionError<T>>`...
This isn't a great place for it either, as we're universally printing to STDOUT, but it's marginally better than where it was before. I'm debating whether or not I should just inject a `Write` to the `Migration`.
We don't just want to have a bunch of directories with meaningless timestamps, as it becomes difficult to know what to edit/read. This also backfills some missing tests for pieces which can be tested in isolation.
There were many cases where we would try to run a migration from a directory that doesn't have the proper structure. This adds tests and changes the behavior to ensure everything is set up properly. What I really want to do is compare all the file names to `["up.sql", "down.sql"]`, but it seems to be more difficult to do that than what we have now.
This is already an implicit contract, so let's make it explicit through our types.
Concerns were raised about enforcing an integer here, and it seems like we'd prefer the flexibility of strings. While I was on the fence about this, I was recently told a story about someone trying to do millisecond precision on migrations in Rails that were generated by a script. This lead to `rake db:rollback` always rolling back the millisecond migration, since it won on integer ordering, even though it didn't win on string ordering. This sold me on just going with strings, even if it allows some strange behaviors.
Now that we've changed the version to be a string, rather than a number, I do want some sort of semantics about what is or isn't a migration, and what's valid in the same directory. With these rules, all files in the directory that aren't part of a migration must start with a `.`.
PSA to those who tried this branch -- delete and recreate your database, there's some schema changes that won't work otherwise. |
This is still missing docs, and I want to flesh the feature out before we ship it, but this is a pretty reasonable sized chunk that I think is ready to be merged on its own.
This adds the ability to manage the database schema by running migrations. Migrations exist in a
/migrations
directory. Each migration is a timestamped sub directory (like20151220_create_stuff
) containingup.sql
anddown.sql
.When a migration is run,
up.sql
will be executed. There is currently no way to revert a migration, though most of the infrastructure for it is in place, and it will rundown.sql
. An internal table is maintained to ensure that migrations are only run a single time.Migrations can be run using the
run_pending_migrations
function, which is meant to be run from a build script. One downside to this is that any console output will get eaten unless the thread panics. I eventually intend to add some command line utilities for these tasks, including generating new migrations.The migrations themselves have no specific integration tests, other than the fact that they are used by our integration test suite to set up the schema. This gives a good smoke test on CI, but unfortunately means that the database needs to be dropped locally if this code changes, to ensure the migrations are re-run. We should eventually improve this.