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

Do not force creation of _cargo-index repo on publish #27266

Merged
merged 7 commits into from
Oct 24, 2023

Conversation

merlleu
Copy link
Contributor

@merlleu merlleu commented Sep 25, 2023

Hello there,
Cargo Index over HTTP is now prefered over git for package updates: we should not force users who do not need the GIT repo to have the repo created/updated on each publish (it can still be created in the packages settings).

The current behavior when publishing is to check if the repo exist and create it on the fly if not, then update it's content.
Cargo HTTP Index does not rely on the repo itself so this will be useless for everyone not using the git protocol for cargo registry.

This PR only disable the creation on the fly of the repo when publishing a crate.

This is linked to #26844 (error 500 when trying to publish a crate if user is missing write access to the repo) because it's now optional.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 25, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 25, 2023
@silverwind
Copy link
Member

silverwind commented Sep 25, 2023

I noticed in a instance of mine that a _cargo-index repo was created, even though I never published a cargo package (I did publish other packages though). Will this get rid of this creation?

@merlleu
Copy link
Contributor Author

merlleu commented Sep 25, 2023

I noticed in a instance of mine that a _cargo-index repo was created, even though I never published a cargo package (I did publish other packages though). Will this get rid of this creation?

Hello, from what I understood only 3 actions can trigger the repo creation: cargo publish or use of the Settings > Packages > Initialize/Rebuild Index.
Are you sure you did not use the 2 last buttons ? Might be worth creating a separate issue including commit log and the logs around the commits if you did not.

@silverwind
Copy link
Member

Ah, yes it might have been the Initialize/Rebuild Index button. Sorry, it's off-topic here :)

@lunny lunny requested a review from KN4CK3R September 27, 2023 07:35
@lunny lunny added the backport/v1.21 This PR should be backported to Gitea 1.21 label Sep 27, 2023
@lunny lunny added this to the 1.22.0 milestone Sep 27, 2023
@lunny
Copy link
Member

lunny commented Oct 2, 2023

Could we know the protocol of the client side? maybe we should obey the client side protocol to do that. Sparse or not.
Or maybe we can retire old protocol entirely.

@merlleu
Copy link
Contributor Author

merlleu commented Oct 2, 2023

Could we know the protocol of the client side? maybe we should obey the client side protocol to do that. Sparse or not. Or maybe we can retire old protocol entirely.

We can't know this this unfortunately, I think we should push users toward using exclusively httpregistry, maybe remove all instructions to setup git registry and make it optional, then disable it entirely in a future version ?

@lunny
Copy link
Member

lunny commented Oct 3, 2023

Could we know the protocol of the client side? maybe we should obey the client side protocol to do that. Sparse or not. Or maybe we can retire old protocol entirely.

We can't know this this unfortunately, I think we should push users toward using exclusively httpregistry, maybe remove all instructions to setup git registry and make it optional, then disable it entirely in a future version ?

I would like the idea. As this PR, maybe we can have an option to enable/disable git registry. Maybe the default value could be false. So spare protocol could be the default one but git registry is optional.

@merlleu
Copy link
Contributor Author

merlleu commented Oct 3, 2023

Could we know the protocol of the client side? maybe we should obey the client side protocol to do that. Sparse or not. Or maybe we can retire old protocol entirely.

We can't know this this unfortunately, I think we should push users toward using exclusively httpregistry, maybe remove all instructions to setup git registry and make it optional, then disable it entirely in a future version ?

I would like the idea. As this PR, maybe we can have an option to enable/disable git registry. Maybe the default value could be false. So spare protocol could be the default one but git registry is optional.

I think this is basically what this PR does: users will have to click "Init Index" in the org/user package settings to have the registry enabled. They can just remove the repo if they no longer want to use git registry and it should not create it again.

Please note that the authorization support for the cargo registry just made it to the nightly branch (on the client) so there should not be too much gitea users using this package registry yet, so we can probably remove the git version before too much people start using it.

Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the method name from AddOrUpdatePackageIndex to just UpdatePackageIndex then? (or a better name? TryUpdatePackageIndex, UpdateExistingPackageIndex, ...?)

services/packages/cargo/index.go Outdated Show resolved Hide resolved
merlleu and others added 2 commits October 8, 2023 22:03
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Oct 8, 2023
@merlleu
Copy link
Contributor Author

merlleu commented Oct 8, 2023

Could you change the method name from AddOrUpdatePackageIndex to just UpdatePackageIndex then? (or a better name? TryUpdatePackageIndex, UpdateExistingPackageIndex, ...?)

Thanks for the feedback, I renamed it to UpdatePackageIndexIfExists.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 9, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 24, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 24, 2023
@lunny lunny enabled auto-merge (squash) October 24, 2023 01:43
@lunny lunny merged commit 796ff26 into go-gitea:main Oct 24, 2023
25 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 24, 2023
Hello there,
Cargo Index over HTTP is now prefered over git for package updates: we
should not force users who do not need the GIT repo to have the repo
created/updated on each publish (it can still be created in the packages
settings).

The current behavior when publishing is to check if the repo exist and
create it on the fly if not, then update it's content.
Cargo HTTP Index does not rely on the repo itself so this will be
useless for everyone not using the git protocol for cargo registry.

This PR only disable the creation on the fly of the repo when publishing
a crate.

This is linked to go-gitea#26844 (error 500 when trying to publish a crate if
user is missing write access to the repo) because it's now optional.

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Oct 24, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 24, 2023
* giteaofficial/main:
  Do not force creation of _cargo-index repo on publish (go-gitea#27266)
  Upgrade to golangci-lint@v1.55.0 (go-gitea#27756)
  Fix incorrect "tab" parameter for repo search sub-template (go-gitea#27755)
  Fix label render containing invalid HTML (go-gitea#27752)
  Fix duplicate project board when hitting `enter` key (go-gitea#27746)
  Fix `link-action` redirect network error (go-gitea#27734)
lunny pushed a commit that referenced this pull request Oct 24, 2023
Backport #27266 by @merlleu

Hello there,
Cargo Index over HTTP is now prefered over git for package updates: we
should not force users who do not need the GIT repo to have the repo
created/updated on each publish (it can still be created in the packages
settings).

The current behavior when publishing is to check if the repo exist and
create it on the fly if not, then update it's content.
Cargo HTTP Index does not rely on the repo itself so this will be
useless for everyone not using the git protocol for cargo registry.

This PR only disable the creation on the fly of the repo when publishing
a crate.

This is linked to #26844 (error 500 when trying to publish a crate if
user is missing write access to the repo) because it's now optional.

Co-authored-by: merlleu <r.langdorph@gmail.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
Hello there,
Cargo Index over HTTP is now prefered over git for package updates: we
should not force users who do not need the GIT repo to have the repo
created/updated on each publish (it can still be created in the packages
settings).

The current behavior when publishing is to check if the repo exist and
create it on the fly if not, then update it's content.
Cargo HTTP Index does not rely on the repo itself so this will be
useless for everyone not using the git protocol for cargo registry.

This PR only disable the creation on the fly of the repo when publishing
a crate.

This is linked to go-gitea#26844 (error 500 when trying to publish a crate if
user is missing write access to the repo) because it's now optional.

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants