Fix removing some packages will result dirty data #34709
Conversation
GiteaBot
added
the
lgtm/need 2
lunny
added
type/bug
backport/v1.24
github-actions
bot
added
lgtm/need 2
|
I don't think this is cleaner than my PR. With this change some package types must call the remove method from the router, other types must call the remove method from the service. |
Your implementation will still have a chance to fail with dirty data. The |
| // DeletePackage deletes the specific generic package. | ||
| func DeletePackage(ctx *context.Context) { | ||
| err := packages_service.RemovePackageVersionByNameAndVersion( | ||
| err := common.RemovePackageVersionByNameAndVersion( |
There was a problem hiding this comment.
Why the common is in "routers" package but not "services"?
There was a problem hiding this comment.
If you need to resolve the dependency problem, it should be something like services/packagemanager to manage all packages.
|
|
||
| for _, pv := range pvs { | ||
| if err := packages_service.RemovePackageVersion(ctx, ctx.Doer, pv); err != nil { | ||
| if err := common.RemovePackageVersion(ctx, ctx.Doer, pv); err != nil { |
There was a problem hiding this comment.
It calls RemovePackageVersion mutiple times and "build" again and again. It doesn't seem right.
It should introduce a final step to rebuild when the removal finishes.
|
Don't backport since the design still has problems and no test. |
|
Simply removing packages by version may cause issues, as identical versions can contain files residing in different groups (refer to #26984), governed by gitea/models/packages/package_file.go Line 40 in 04017f2 For instance, package A real-world case: BTW cleanup functionality is actually implemented twice in the codebase: This duplication may cause discrepancies between the preview results and actual execution outcomes. |
5 participants
Fix #32830
Inspired by #22810 and replace it.
When deleting alphe, cargo, debian, rpm packages, some extra actions should be taken which was missed. It will result in dirty data and inconsistent. This PR fix it. To avoid recycle dependencies, this is a quick patch. Like #22810 (comment) said, this might need a new design for such more packages types.