Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform actions after a package was removed #22810

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Feb 7, 2023

The removal of packages outside of the defined package registry flow may lead to dirty data if the package type stores informations about the available packages outside of the database. For example the Cargo registry stores the package index in a git repository. At the moment the owner needs to rebuild the whole index in this case from the user settings.

This PR enforces an update of the index entry in the git repository. In future the Debian registry will need this too.

@KN4CK3R KN4CK3R added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/packages labels Feb 7, 2023
@KN4CK3R KN4CK3R added this to the 1.20.0 milestone Feb 7, 2023
@delvh
Copy link
Member

delvh commented Feb 7, 2023

I do have to say, the current architecture looks pretty error-prone:
Every caller must ensure themselves that they call the post-removal.
Sooner or later, this will produce buggy and unmaintainable behavior, simply because someone forgets it.
I think it would be a better idea to define an interface Registry (or similar) that gets the methods that every registry needs, i.e. GetPackage, GetPackages, UploadPackage, EditPackage(not sure if that's supported), and DeletePackage (did I forget any common method?).
Then, you define an implementation per registry.
That way, adding new registries (and maintaining the existing ones) will be much easier as you only need to define such an implementation and can throw out a lot of code duplication.
Adding all that is certainly worth its own PR.


I think at the moment, you're the only one who really knows what's happening where inside the registry-related code.
That's not maintainable and a way too low bus factor.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 7, 2023
@zeripath
Copy link
Contributor

zeripath commented Feb 7, 2023

Why doesn't the packages_service run the cleanup function itself? Isn't it fragile to expect callers of packages_service.Remove... to remember to do the cleanup?

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 8, 2023

We must distinguish between two different deletion modes:

The first is specified by the package type registry api. If there is a designated delete method there is no problem because the deletion of package is an intended operation.

The second is our custom delete operation from the UI or API. This operation is not covered by the package type specification. Most package types are ok with this because the client just queries the available packages and now there is one missing, everything fine. But there are also package types where it is not possible to generate such query responses on the fly. An example is the Cargo registry which stores the package index in a git repository. If a package gets deleted from the UI/API the git repository must be changed too, otherwise the repository is out of sync with the db. Currently there is only a note in the Gitea documentation what to do in this case.

So this PR is only needed for the second unsupported case. There are only 4 places where such logic is needed. Deletion from the package UI, deletion from the admin UI, deletion from the package API (Gitea API, not the registry API) and deletion by a cleanup rule.

I think it would be a better idea to define an interface Registry (or similar) that gets the methods that every registry needs

That's a naïve view of how most package registries work and only a few will fit in. The common "interface" all registries (except the uploading of container images) use, are the methods in services/packages/packages.go. Everything in the router is registry type specific. From my view there is not much left which is duplicated.

I think at the moment, you're the only one who really knows what's happening where inside the registry-related code.

I will add a readme file for future developers to explain how everything works together. (We should have more of these rough overviews of how things work in Gitea. But there should not be to much detail so they are not outdated shortly after.) As seen in #20751 (comment) it looks like the interface is well thought out, otherwise it would not have been possible to add a new package type without asking questions.

Why doesn't the packages_service run the cleanup function itself? Isn't it fragile to expect callers of packages_service.Remove... to remember to do the cleanup?

I thought of multiple ways on how to implement it. The first guess is to add it into RemovePackageVersion which also handles the webhook notification. The first problem would be an import cycle there but that may be handled otherwise. The more important problem is that this method is used by the both deletion modes (defined and undefined). For the defined deletion this method should not perform the post-delete actions because they may (and are) different from the actions to be performed by the undefined deletion. So there could be a parameter to enable the actions but that's just again the "expect the callers" problem. I could add a RemovePackageVersionUndefined method which first calls RemovePackageVersion and then performs the actions. So the actions can't be forgotten but the caller must still use the correct method then. But I don't see a problem here generally because currently I can't think of a new way which would need unsupported deletes.

techknowlogick pushed a commit that referenced this pull request Mar 13, 2023
As announced in #22810 I added a readme file to help understanding how
the package registry archictecture works and how the go packages are
related.
routers/web/user/package.go Outdated Show resolved Hide resolved
@delvh delvh removed this from the 1.20.0 milestone Jun 4, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Nov 12, 2023
@lunny
Copy link
Member

lunny commented Apr 3, 2024

Can we have some tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/packages type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants