Migrate to Xormigrate

[qwerty287]

Migrate the migration handling to xormigrate instead of the native handling.

The main difference between the methods: gitea has a version-based system, xormigrate uses id-based. This means: In theory, each migration could be executed standalone (xormigrate will however execute them in order).

This allows backporting migrations, and xormigrate is already in use in projects like woodpecker ci (and is working well there).

[techknowlogick]

Thanks @qwerty287! I need to reply to your email, but thanks for getting started on this.

[qwerty287]

I think this is ready for a first review round. Tests pass both in CI and also in my manual tests, everything seems to work.

@techknowlogick

[qwerty287]

@wxiaoguang please check out #31523 (comment) for some answers to your questions

[wxiaoguang]

@wxiaoguang please check out #31523 (comment) for some answers to your questions

My opinions:

1. "rollback" is not practicable (actually it is not right in my mind and I would never do "rollback" for a app's database migration), so I agree.
2. Not only "deleting", there could be some "data fix", "default value" or other problems.

For example:

1. Should a data fix PR be backported to 1.19.3? Then 1.20.0 doesn't recognize it and reports 500 error?
2. If a newly added column needs some "default value" provided by 1.19.3 (from code), then 1.20.0 would insert wrong default values (from database), then when user goes to 1.20:latest, then 500 error?

The "backporting" problem is more complicated that it looks, just like you said: "We usually don't do backports at woodpecker anymore."

So " if there is no significant benefits (no need to do so), why take the risk to touch the stable existing migration mechanism?"

[qwerty287]

So " if there is no significant benefits, why take the risk to touch the stable existing migration mechanism?"

Well, from the comment above:

One of the biggest benefits is that our migrations can align with migrations approaches in many other software offerings (rails, laravel, and many more) where instead of relying on a single int, all of the migrations that have been run are saved into the DB. We also don't need to no-op migrations, as we only had to do that because we relied on an int to be the same (allows for code cleanliness). We could also have doctor tasks run on prior versions without having to require a user to manually run something in the CLI.

[wxiaoguang]

One of the biggest benefits is that our migrations can align with migrations approaches in many other software offerings (rails, laravel, and many more) where instead of relying on a single int, all of the migrations that have been run are saved into the DB.

It looks quite trivial to me because Gitea already have heavily customized a lot of modules, include http/web/router/logger/queue/template/frontend, etc. Nothing is Gitea is really like "other software" at the moment. So making the migration system like or unlike others doesn't really matter.

We also don't need to no-op migrations, as we only had to do that because we relied on an int to be the same (allows for code cleanliness).

Why it ever needed to "no-op migrations"? As long as there is no backporting.

We could also have doctor tasks run on prior versions without having to require a user to manually run something in the CLI.

It also involves "backporting", which is the most risky (unclear) part in this mechanism.

[qwerty287]

Why it ever needed to "no-op migrations"? As long as there is no backporting.

That's actually a good question, but to me it seems some migration were removed, maybe because they're not necessary anymore. There are a lot of no-ops currently in the code, and removing them is not possible because of the int-based migration system. That would not be a problem with xormigrate: Removing a migration is possible without any problems due to its name-based system.

[wxiaoguang]

These no-op are mainly related to fix some legacy problems, but not related to backporting, instead, they have a hard dependency of the following migration, so, literally they are "replaced". Actually they do not need to be really removed (no-op), keeping the old code there would still be fine (just some duplicate or unnecessary changes).

For example, it ever added a table, then dropped that table. When using a name-based migration system, there should be a separate step to "remove the migration ID from database", otherwise there will be an unknown migration ID in database. It couldn't be simpler than "no-op".

[qwerty287]

When using a name-based migration system, there should be a separate step to "remove the migration ID from database"

This could be directly added into xormigrate to have an automatic "cleanup" of this table.

[wxiaoguang]

After a quick code read, I think this PR's change is not mature (so if let me decide, I would also say "block" for it).

If we would really like to do it:

• It needs a complete design and document (code comments) first.
• Support backporting or not, why/how (edge cases: use 1.19.3 with backport from 1.20.3 -> use 1.20.0)
• Some edge cases need to be clarified, for example: run an old Gitea with a new database, what happens.
  • Would the old Gitea run? Then mismatched database would cause various problems (it happened a lot in history)
  • Would the old Gitea refuse to run? Then how could developers switch between 1.19 and 1.20 for development.
    • Old code shows an "UPDATE version = ..." hint, could easily force downgrading (for development only)
• What happens when there are unknown migration IDs in database?
• x.Insert(&xormigrate.Migration{ID: i}) doesn't seem right. According to xormigrate, the old migration name should be filled into Desc field, while ID field should have some real ID-like values.
• It needs a sample to show how to write new migrations and needs some test code to cover the changes (convert from the old migration mechanism, edge cases)

---

But, before starting the work, I would like to see people have the consensus first: is it really worth to introduce xormigrate? (at the moment, my answer is no because the benfits seem quite trival to me, the potential risks(bugs/regressions) might be more than benefits)

[wxiaoguang]

Migrate to Xormigrate or not, we need this first: Refactor the DB migration system slightly #32344

By #32344 , each migration has a clear id number, it does help the problem in #31523 (comment):

x.Insert(&xormigrate.Migration{ID: i}) doesn't seem right. According to xormigrate, the old migration name should be filled into Desc field, while ID field should have some real ID-like values.

[techknowlogick]

As an update, I am starting a discussion with the other maintainers regarding this PR and possibly creating a formal RFC. There are the concerns above, and I hear/understand them. Our contribution document allows all maintainers to have a voice, which I hope will be able to feed into an RFC to allow for a robust approach that satisfies everyone.

Another big thank you to @qwerty287 for their work on his PR already.

[qwerty287]

Hey @techknowlogick, are there any news about this?

[lunny]

Hey @techknowlogick, are there any news about this?

I think we can close this PR and also close the bounty before we have finished the discussion. But since @qwerty287 has put in a lot of effort on this PR, it's fair to award him as well. @techknowlogick

[lunny]

Thank you for your effort. Given that this issue is far more complicated than we initially expected, it is not feasible to address it through the bounty program. Consequently, we have decided to remove it from the bounty list and close this PR.

[jolheiser]

The prior two comments in this thread appear to contradict each other, maybe they just need clarification?
Is @qwerty287 being compensated for the work put in for this PR? It reads like this work was initially sponsored.

[techknowlogick]

@jolheiser they will be fully compensated.

@qwerty287 thank you again for all the work you put into this. After discussion with others, they have (very patiently) detailed why my approach and library would impact the project in a way that could cause a negative experience for the users. I was focused on one aspect, where there would be benefits without considering other. This is most certainly not a reflection on you or your ability as you executed my vision flawlessly, and the code you provided was top notch. I think I had a hammer in search of a nail, and saw how well it worked for you with woodpecker that I wanted to bring it over here. I think I in the future I will draw up a proposal and discuss with others prior to any large refactoring. Usually things like this are discussed, and I had casually talked about it, but for this change definitely not enough.

[techknowlogick]

@jolheiser also to clarify Lunny's comment, he meant to remove it as an option for others to claim a bounty on it. In the case of this specific PR I had reached out to @qwerty287 and used the bounty system as a way for transferring funds as the tax man needs a receipt pretty much for all my expenses, so rather than foist that upon qwerty287 I opted to use the platform to make everything less painful

[qwerty287]

Thank you for explaining the details @techknowlogick and @lunny.

@techknowlogick I sent you an email about the bounty.

[algora-pbc]

@qwerty287: You've been awarded a $700 bounty by Gitea! 👉 Complete your Algora onboarding to collect the bounty.

[algora-pbc]

🎉🎈 @qwerty287 has been awarded $700! 🎈🎊

• Share on socials
• Give feedback