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

Don't run push mirrors for archived repos #27140

Merged
merged 8 commits into from
Jan 24, 2024
Merged

Conversation

JakobDev
Copy link
Contributor

Fixes https://codeberg.org/forgejo/forgejo/issues/612

At the moment push mirrors are still run if a repo is archived. This PR fixes this.

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

KN4CK3R commented Sep 19, 2023

Don't know if that is really not the preferred behaviour. If you add a new push mirror for an archived repo I would expect that the mirrored-to repo gets updated.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 20, 2023

Considering an archived repo is read-only, I don't see it should support adding more push mirrors or do the push.

I am neutral for this change, but the nit is that I think the "check" could be done in the "handler" (after m.GetRepository()), then the SQL could still be kept simple, and the code could be easier to read and it's easier to add test code for it (maybe in the future). (hmm, no idea whether it is really better).

Another question is that what's the behavior of the "pull mirror"? Should it pull for archived repo?

@lunny
Copy link
Member

lunny commented Sep 20, 2023

I aslo think it's unnecessary to push to upstream when a repository has become archived one. Since there will be no new commits pushed to this archived repository and it will always keep the last commits not changed. Why it should keep to push to remote? It's wasting cpu and memory.

I think the right way to fix this is to disable the buttons on the UI and API to disallow change the push mirrors information. These push mirrors remote addresses should not be removed because maybe the repositories will be converted back to common mirror repositories.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 20, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Sep 20, 2023
@JakobDev
Copy link
Contributor Author

I think the right way to fix this is to disable the buttons on the UI and API to disallow change the push mirrors information.

Done

grafik

but the nit is that I think the "check" could be done in the "handler" (after m.GetRepository()), then the SQL could still be kept simple, and the code could be easier to read and it's easier to add test code for it (maybe in the future).

I think the SQL is simple enough. Joins are just basic SQL that everyone who knows SQL should understand. We should not load more from the Database than needed.

Another question is that what's the behavior of the "pull mirror"? Should it pull for archived repo?

You can't archive a pull mirror.

grafik

@lunny
Copy link
Member

lunny commented Sep 24, 2023

I think the right way to fix this is to disable the buttons on the UI and API to disallow change the push mirrors information.

Done

grafik

but the nit is that I think the "check" could be done in the "handler" (after m.GetRepository()), then the SQL could still be kept simple, and the code could be easier to read and it's easier to add test code for it (maybe in the future).

I think the SQL is simple enough. Joins are just basic SQL that everyone who knows SQL should understand. We should not load more from the Database than needed.

Another question is that what's the behavior of the "pull mirror"? Should it pull for archived repo?

You can't archive a pull mirror.

grafik

Could we just disable the mirrors' buttons but keep the information displayed?

wxiaoguang
wxiaoguang previously approved these changes Sep 24, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

I would suggest not indenting the {{else}} block, to avoid unnecessary conflicts.

@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 Sep 24, 2023
@6543
Copy link
Member

6543 commented Oct 1, 2023

Please resolve conflict :)

@6543 6543 added this to the 1.22.0 milestone Oct 1, 2023
@wxiaoguang wxiaoguang dismissed their stale review October 24, 2023 01:46

stale for long time, so have to dismiss the approval.

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 24, 2023
models/repo/pushmirror.go Outdated Show resolved Hide resolved
@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 Jan 11, 2024
@denyskon
Copy link
Member

@JakobDev Please resolve conflicts :)

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2024
@pull-request-size pull-request-size bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 23, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2024
@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 Jan 23, 2024
@denyskon denyskon added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 23, 2024
@lunny lunny enabled auto-merge (squash) January 24, 2024 02:12
@lunny lunny added backport/v1.21 This PR should be backported to Gitea 1.21 and removed backport/v1.21 This PR should be backported to Gitea 1.21 labels Jan 24, 2024
@lunny
Copy link
Member

lunny commented Jan 24, 2024

This PR is not easy to be backport to v1.21, so I just removed the label.

@lunny lunny merged commit f3ba3e9 into go-gitea:main Jan 24, 2024
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 24, 2024
@JakobDev JakobDev deleted the noarchivepush branch January 24, 2024 07:15
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 25, 2024
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Show in Web UI if file is vendored and generated (go-gitea#28620)
  Don't reload timeline page when (un)resolving or replying conversation (go-gitea#28654)
  Allow to sync tags from admin dashboard (go-gitea#28045)
  Don't run push mirrors for archived repos (go-gitea#27140)
  Only migrate the first 255 chars of a Github issue title (go-gitea#28902)
  Fix hardcoded GitHub icon used as migrated release avatar (go-gitea#28910)
  [skip ci] Updated translations via Crowdin
  Update go dependencies and fix go-git (go-gitea#28893)
henrygoodman pushed a commit to henrygoodman/gitea that referenced this pull request Jan 31, 2024
Fixes https://codeberg.org/forgejo/forgejo/issues/612

At the moment push mirrors are still run if a repo is archived. This PR
fixes this.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
Fixes https://codeberg.org/forgejo/forgejo/issues/612

At the moment push mirrors are still run if a repo is archived. This PR
fixes this.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants