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

[lunny]

I believe we currently lack sufficient motivation to change our migration method.

[qwerty287]

@lunny we currently have at least 5 people in favor of this change (@techknowlogick and @delvh + 2 more from PR reactions + me) while you're the only one argumenting in the other direction

[qwerty287]

@lunny @techknowlogick Please review code and discussion. It's really frustrating seeing PRs like this ignored. Just think of people contributing the first time - you think that increases the probability that they'll continue contributing to open source?

[wxiaoguang]

Well, it happens a lot. See my "pings" of these screenshots.

---

---

---

I guess there will be some new "sorrys" for this PR. And it is always a shame that some of the TOC team do not fulfill the responsibilities.

---

So , to be honest, at the moment, either:

1. Become a maintainer and help to improve and approve more PRs, and actively fix and push things, instead of waiting for others or leaving code to others.
2. Be patient to persuade others and use various proofs to prove the correctness and necessity
3. Ask ahead for promise of the approvals and only make widely-accepted changes.

[wxiaoguang]

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

My concern is still not addressed: #31523 (comment) (even it is not documented ..... that's a risk)

I don't know how woodpecker and Vicuna resolves these problems?

We usually don't do backports at woodpecker anymore.

So , if "don't do backports", then I guess the PR doesn't fulfill its purpose "This allows backporting migrations", if there is no significant benefits, why take the risk to touch the stable existing migration mechanism?

---

I do not mean saying "no", I just want to make everything clear, to get rid of risks. It happens a lot in history that a "good-looking mechanism" would leave some hard-to-fix problems or regressions.

[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.