gitea
gitea copied to clipboard
go-gitea
Migrate to Xormigrate
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).
Thanks @qwerty287! I need to reply to your email, but thanks for getting started on this.
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
Could you add description about
- The comparasion between the two migrations method.
- Why should we switch?
- Is it safe enough for a smooth upgrade? Could you add some tests for the upgrade includes a fresh Gitea installation and an upgrade operation.
@lunny, I have sponsored @qwerty287 to do this work. We should switch because it means we can backport migrations, and this library is already in use with the Woodpecker and Vicuna projects (projects that Gitea maintainers are also involved with/lead).
@lunny, I have sponsored @qwerty287 to do this work. We should switch because it means we can backport migrations, and this library is already in use with the Woodpecker and Vicuna projects (projects that Gitea maintainers are also involved with/lead).
Yes. But we still need more description on the issue content so that there are enough commit messages once this is merged.
I updated the PR desc, can you check it? If something is missing, feel free to just update it.
About the tests: They exist already (and pass).
It seems that it goes back to history again.
Some concerns are not really resolved or mentioned (related issue/PR: #23511 , #23078 , #10625 , #8613 )
For example:
- https://github.com/go-gitea/gitea/pull/23078#issuecomment-1440290694
Assume users are in v1.19.3 which will add a new column, but for v1.20.0 has no migration for this column but v1.20.3 has, when user upgrade from v1.19.3 to v1.20.0 not v1.20.3, it will encounter problems.
And one more thing, backporting database changes might break the compatibility promise.
To be clear, I do not mean "blocking". I only mean these edge cases should be clearly answered and documented. For example, what could be backported, what would happen, what should be prohibited from backporting.
https://docs.gitea.com/installation/upgrade-from-gitea#backup-for-downgrade
Migrations in fact have dependencies, I think we need to indicate the dependencies for all old and new migrations. And migrations cannot exist standalone actually. The dependencies need to be defined.
Migrations in fact have dependencies, I think we need to indicate the dependencies for all old and new migrations.
New migrations, yes, old ones, not necessary as they are probably not being backported.
I don't think this is something that must be directly in the code, only in a comment. Xormigrate does not need to know about this, migrations are executed in the order they're defined.
How should I handle this?
Migrations in fact have dependencies, I think we need to indicate the dependencies for all old and new migrations.
New migrations, yes, old ones, not necessary as they are probably not being backported.
I don't think this is something that must be directly in the code, only in a comment. Xormigrate does not need to know about this, migrations are executed in the order they're defined.
How should I handle this?
Even a new migration will have dependency, imagine two migrations will change the same tables, one is creating a column another is change the column type. So that the later one will depend on the first one in logic. It will be very easy to miss that dependency when backport migrations. And this is a very easy example and I think there will be much more complexity ones. How will this PR/xormigrate handle the situations?
Even a new migration
That's what I wrote? It's not necessary for old ones and therefore not that important for this pr. Adding a new migration later needs to pay attention to this.
How will this PR/xormigrate handle the situations?
Xormigrate doesn't do something on this direction, and I don't think it has to.
Xormigrate executes the migrations in a fixed order, so adding migration dependencies before another one will execute them first. It must be documented somewhere which dependencies are necessary and need to be backported as well, but this everything I'd say.
Assume we backport a migration to a stable version. Two problems need to be considered.
- Does this backported migration have dependency migrations that haven't been backported? It's better to have clear dependencies on the code but not according to our brain which can reduce possible backport problems.
- Once 1.xx.2 merged a backported migration, how user downgrades to 1.xx.1 like @wxiaoguang said? It's very important once 1.xx.2 have some big bugs for users. It's also a compatible promise. So maybe a rollback migration needs to be supported?
I don't know how woodpecker and Vicuna resolves these problems?
So maybe a rollback migration needs to be supported?
Xormigrate supports this. It can be added as a separate function on the migration.
I don't know how woodpecker and Vicuna resolves these problems?
We usually don't do backports at woodpecker anymore.
With backports, we could do more than modify the database; we could add doctor commands to be run, so we still meet the requirement of being changeable between minor versions.
With backports, we could do more than modify the database; we could add doctor commands to be run, so we still meet the requirement of being changeable between minor versions.
Since we have a web UI for administration, I think it's better to let the user do a self-check there and do a fix there. But not fix them automatically because some doctor commands may take a long time.
In summary, I couldn't find the necessary and obvious benefits to migrate to Xormigrate. It cannot resolve the backports in minor versions.
It cannot resolve the backports in minor versions.
It cannot do that alone obviously, because if there's a dependency of a migration defined, it must be backported as well. Xormigrate is not an automation tool that does this, the devs still have to do this. Implementing strict dependency checking in xormigrate is not a big problem and could help here - if I should follow this approach let me know.
It cannot resolve the backports in minor versions.
It cannot do that alone obviously, because if there's a dependency of a migration defined, it must be backported as well. Xormigrate is not an automation tool that does this, the devs still have to do this. Implementing strict dependency checking in xormigrate is not a big problem and could help here - if I should follow this approach let me know.
I think my concerns are still here which ask the migrations system support both dependency and rollback. And I can't guarantee the rollback can always work because some migrations are unrevertable. So that will still break the compatibility.
Does this backported migration have dependency migrations that haven't been backported? It's better to have clear >dependencies on the code but not according to our brain which can reduce possible backport problems. Once 1.xx.2 merged a backported migration, how user downgrades to 1.xx.1 like @wxiaoguang said? It's very important >once 1.xx.2 have some big bugs for users. It's also a compatible promise. So maybe a rollback migration needs to be >supported?
Does this backported migration have dependency migrations that haven't been backported? It's better to have clear dependencies on the code but not according to our brain which can reduce possible backport problems.
That's what I said above. Currently xormigrate does not support this, but I can implement it there.
Once 1.xx.2 merged a backported migration, how user downgrades to 1.xx.1 like @wxiaoguang said? It's very important once 1.xx.2 have some big bugs for users. It's also a compatible promise. So maybe a rollback migration needs to be supported?
Xormigrate supports rollbacks. You would probably need some cli command to perform this.
@lunny what about my suggestion above? https://github.com/go-gitea/gitea/pull/31523#issuecomment-2320403257
Does this backported migration have dependency migrations that haven't been backported? It's better to have clear dependencies on the code but not according to our brain which can reduce possible backport problems.
That's what I said above. Currently xormigrate does not support this, but I can implement it there.
Once 1.xx.2 merged a backported migration, how user downgrades to 1.xx.1 like @wxiaoguang said? It's very important once 1.xx.2 have some big bugs for users. It's also a compatible promise. So maybe a rollback migration needs to be supported?
Xormigrate supports rollbacks. You would probably need some cli command to perform this.
Thanks for the effort to push the pull request. I think even if both of these two features are implemented. Backporting migrations to a stable version is still dangerous. Most of the time, rollback cannot work well. Assume we delete a column in v1.22.1, I don't know how it can be rolled back when we are v1.22.2. The column data has been missed, it cannot be rolled back in real. If this pull request cannot bring real benefit but more risk, I don't know how should I agree to merge this.
Taking a look at Golang which Gitea has a similar versioning, I don't think they will have a traditional LTS version plan.
@lunny we don't need to add rollback support if this library is included, it is not mandatory. We would also need to ensure that if any migrations are backported, they still meet our upgrade/downgrade on minor version compatibility promises.
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.
Assume we delete a column in v1.22.1, I don't know how it can be rolled back when we are v1.22.2. The column data has been missed, it cannot be rolled back in real. If this pull request cannot bring real benefit but more risk, I don't know how should I agree to merge this.
Deleting a column is already possible in the existing code on minor versions, the only thing that is stopping that is those PRs would be viewed by a maintainer and blocked. So we would keep those same policies, and if a migration that doesn't meet the upgrade/downgrade policy on minor versions it would be blocked.
@lunny I'd like to get that done. Could you check out @techknowlogick's comment?
I cannot get what's the benefits. Oppositely, the conversion itself is full of risk, and allowing backport migrations is also full of risk because contributors/code reviewers need to remember the dependencies between migrations. I know the change will allow us to fix some bugs with migrations in stable minor versions. But I think the situation is very rare.
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.
contributors/code reviewers need to remember the dependencies between migrations
Sorry to get a bit impolite here, but did you actually read my comments?!
Implementing strict dependency checking in xormigrate is not a big problem and could help here - if I should follow this approach let me know.
You don't need to remember that if I implement something like this in xormigrate.
@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
@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?
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:
- 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.
- Be patient to persuade others and use various proofs to prove the correctness and necessity
- Ask ahead for promise of the approvals and only make widely-accepted changes.
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: https://github.com/go-gitea/gitea/pull/31523#issuecomment-2257664130 (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.