Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.9k
Fix removing some packages will result dirty data#34709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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. |
lunny commentedJun 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Your implementation will still have a chance to fail with dirty data. The |
| @@ -144,7 +145,7 @@ func UploadPackage(ctx *context.Context) { | |||
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why thecommon is in "routers" package but not "services"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you need to resolve the dependency problem, it should be something likeservices/packagemanager to manage all packages.
| @@ -393,7 +394,7 @@ func DeletePackage(ctx *context.Context) { | |||
| } | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It callsRemovePackageVersion 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. |
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.