Supporting custom implementations of state persistance.

[gustavnikolaj]

Ref: #29

This PR seperates the save and load logic from the Set class. The Set can now be instantiated with either a path to the migration file, or a custom implementation of a way to persist state.

The original programmatic use will still work as it always did.

var migrate = require('migrate');
var set = migrate.load('migration/.migrate', 'migration');

But now you can do:

var migrate = require('migrate');
var set = migrate.load({
    save: function () {},
    load: function () {}
}, 'migration');

How to wire this in to the CLI is an open question. I imagine that a new --state parameter could be added which would need to point to a .js file that exported an object that could be used instead of the path, as the argument to the Set constructor.

[gustavnikolaj]

It's released on NPM as @gustavnikolaj/migrate if you want to try it out.

[jimmed]

Obligatory +1 for a merge here

[gregbarcza]

+1

[gustavnikolaj]

ping @LinusU

[gustavnikolaj]

ping @joaosa

[wmartins]

+1

[interisti]

+1

[tommysanterre]

+1

[crunchable]

for some reason the migrations are not stored (only 'pos') .. isn't this necessary later on to calculate which new migrations appeared?

[gustavnikolaj]

I must admit that I don't remember anything about this, but past-me wrote the justification here: gustavnikolaj@0c74d2f

So it seems that it is safe, and that it will give exactly the same functionality as the present implementation, and the same drawbacks as well. If you want to persist more information, it would require changes outside of this part of the code, to make use of it.

[LinusU]

@crunchable Yes that is indeed a bug, or at least poor behaviour. I would really like it to save which migrations have been applied instead of just saving an arbitrary position.

This change should really happen before we merge custom state persistence since that will probably break all custom implementations.

[gustavnikolaj]

It's strictly adhering to the existing implementation, to keep the change set as small as possible and to make sure that it doesn't break backwards compatibility. :-) It's been a while, but I'm sure that I wouldn't sent off a pull request with failing tests.

If you're not confident with the state persistence in the current implementation, we could add some tests of the current behaviour to prove that this PR is not altering that behaviour.

And actually, it will not break unless you do migrations in a weird way. You should treat migrations as an append only list, which makes your index in that list the only interesting information.

[LinusU]

The problem is not with this pull requests, it's with the current functionality of the code. I'm sure that this doesn't break that, but it makes it harder for us to fix since we have to account for all other ways of saving the state.

The problem is if two developers are working in different branches, on different features, and both add one migration each.

[gustavnikolaj]

Of course! It's easier to fix that before allowing people to come with their own implementations.

When that is said, I think that it is impossible to solve this problem in a general way. Consider this case:

Developer A and B both branch out of master with feature branches called feature/a and feature/b respectively. They both add a migration. Developer A merges his branch first. From there, we have two possible cases:

1. Developer B proceeds to merge his branch to master, and run the migrations. The state in the database has all migrations from master, and the one from feature/b already. There's no way of knowing if the result of applying the migration from feature/a on top of the database with master + feature/b is the same result as applying what's on master + feature/a + feature/b.

2. The other option is that Developer B goes on to rebase his branch on top of the new master with feature/a on it already, and make sure that his migration applies cleanly on top of that before merging it.

Option 2 is the only viable one, as there is no way to know if the result of doing those two migrations will be the same. If there is a way, it's going to be specific to database implementations and thus out of scope for this module - and even then, I'd be very cautious of actually relying on something like that.

Thus, the only options we have revolve around forcing/pushing people into choosing option 2. But no matter what we do (expect from forcing people into having non-descriptive migration names - e.g. 001.js, 002.js ...) it will ultimately be based on convention.

If you cannot trust that people are disciplined about using migrations as append-only lists, I really don't think that we can come up with a generic solution.

[gustavnikolaj]

A possibility could be to extend this pluggable state class such that it stored and loaded the entire state passed to it - instead of complying with the old implementation - and also make other parts of the migrate package pluggable. That way someone could make conflict-resolution-strategy plugins for specific databases.

Do you know any other migration frameworks that handle cases with "conflicting" migrations automatically?

[LinusU]

I'm at work so I'll try and read and respond properly later. I think that Ruby's rake migrate has it covered. I think they save datetime, filename, action every time they migrate up or down.

[ktonon]

One possible resolution to the migration conflict problem (i.e. two developers both adding a migration at the same time) is to maintain an index file containing an ordered list of all migrations.

If added to the repo, version control will catch the conflict before any merging happens. It would be simple to extend the create action to maintain this index file.

[romeovs]

+1

[wesleytodd]

I believe this is now all working in #77. This was similar to #67, and I think the implementation I have in the 1.x branch is a bit more similar to that, but it should satisfy both needs. Let me know, and feel free to close this if you are alright with that implementation. All feedback on that work is welcome, so please let me know what you think @gustavnikolaj!

[gustavnikolaj]

@wesleytodd sounds good. I'll just close this PR. It's been dangling for a long while and I don't think I'll have much to add. It's been more than 2 years since I had my head in this stuff :-)