Execute all pending migrations regardless of their timestamp

[Quentinchampenois]

Description

Hello,

At the moment, pending migrations are only executed if the version number is more recent than the last migration version.
This PR aims to find all pending migrations present in filesystem regardless of their timestamp before executing them.

• Fixes All pending migrations should run, regardless of their timestamp  #983

Notes

Feel free to give feedback, ask for changes or share good practice with go 👍

Thanks !

[indyteo]

Disclaimer: I'm not a maintainer of this project, I just wanted to share a small review :)

[indyteo]

I first had trouble understanding why you used a map "version => boolean" but I understood while writing my suggestion. I don't know about performances, but anyway I think this is not a big deal because the bottleneck of this function is the SQL request.
Here is my original suggestion:

Instead of using a temporary map, you can build the final array by removing versions already applied (found in the database).

You need to start from a copy of all versions:

pendingMigrations := slices.Clone(versions)

And then modify the loop as follow:

Suggested change

	for rows.Next() {
	var version int
	_ = rows.Scan(&version)
	dbVersionMap[version] = true
	}
	for rows.Next() {
	var version int
	_ = rows.Scan(&version)
	i := slices.Index(pendingMigrations, version)
	if i != -1 {
	pendingMigrations = slices.Delete(pendingMigrations, i, i + 1)
	}
	}

I think this simplifies a little bit the code

[Quentinchampenois]

I also agree this is more readable, at the beginning, I started to implement the solution using slices package and then I imagined that we don't want to import new package on this file, So I managed to find a solution without importing package 😅

I will refactor this part with slices based on your suggestion 👍

[indyteo]

I'm not a Go specialist, and I don't know the exact impact of importing a package. Because slices is part of the standard library, and I guess is already used somewhere else in the project (or in any dependency), I imagine those functions were already included in the final executable. If not, it may be appropriate to do without them, indeed.

[mattwoberts]

Thanks a lot @Quentinchampenois ! And also thanks for the review @indyteo 👍

I'll take a look at this soon :)

[mattwoberts]

Thanks @Quentinchampenois - made a few suggestions, hopefully they all make sense.

[mattwoberts]

@Quentinchampenois Forgot to mention - would be good if you could include some tests too (there is already a test file that should get you started - shout if you need any more help with this)

[Quentinchampenois]

Hello @mattwoberts thanks for the review and feedbacks !

I applied your suggestions from the change request, and I also added a new test, feel free to share feedback or ask for changes

[mattwoberts]

🎉

Thanks @Quentinchampenois !

[Quentinchampenois]

Nice, thank you both for your time !

[mattwoberts]

@Quentinchampenois

FYI, I've had to revert this for now...

When I deployed the change to our production server, I got loads of errors related to SQL migration files could not be run. When I looked into the existing migrations, there were loads of migrations not recorded in here, that the migrator logic was trying to run.

It looks like the migration table and logic was added later on - so early SQL migrations (2017 / 18) didn't have a log of being run - causing all sorts of havoc.

So - it's reverted out of main for now. I'm not sure what the best approach is to handle this - I suspect there will be many self hosters in a similar situation who would potentially break their instance with this migration code.

[indyteo]

Maybe you should try to get the exact date (based on migrations "versions" which are basically timestamps, by looking at the 1st that got recorded inside your database) at which the migration logic had been implemented, and add a condition here to ignore older migrations (except on 1st run, where all migrations should always run).

The issue will be for instances that ran some migration (but not all) in 2017, before the actual system got implemented (i.e. no migration recorded but some tables found in database). And maybe the solution is just to abort because it requires human maintenance to find out the version of the database of the self-hosted instance... And it will probably not affects a lot of users anyway. Those users would face a similar issue anyway when upgrading to the actual system, because it would try to re-run old migrations as well, right?

[Quentinchampenois]

@mattwoberts sorry to learn it caused troubles !