Skip to content

Option to delay conflict checking of old pull requests until page view #27779

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Apr 24, 2025
Merged

Option to delay conflict checking of old pull requests until page view #27779

merged 14 commits into from
Apr 24, 2025

Conversation

brechtvl
Copy link
Contributor

@brechtvl brechtvl commented Oct 24, 2023 •
edited
Loading

[repository.pull-request] DELAY_CHECK_FOR_INACTIVE_DAYS is a new setting to delay the mergeable check for pull requests that have been inactive for the specified number of days.

This avoids potentially long delays for big repositories with many pull requests. and reduces system load overall when there are many repositories or pull requests.

When viewing the PR, checking will start immediately and the PR merge box will automatically reload when complete. Accessing the PR through the API will also start checking immediately.

The default value of 7 provides a balance between system load, and keeping behavior similar to what it was before both for users and API access. With 0 all conflict checking will be delayed, while -1 always checks immediately to restore the previous behavior.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 24, 2023
@techknowlogick techknowlogick added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Oct 24, 2023
@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 12, 2024
wxiaoguang
wxiaoguang reviewed Sep 12, 2024
View reviewed changes
@github-actions github-actions bot added modifies/go Pull requests that update Go code docs-update-needed The document needs to be updated synchronously labels Sep 12, 2024
@brechtvl
Option to delay conflict checking of old pull requests until page view
dbd6fe1 
[repository.pull-request] CHECK_ONLY_LAST_UPDATED_DAYS specifies the
number of days in which the PR must have been last updated for conflict
checking to happen immediately when the base branch is updated.

The default value of -1 behaves exactly as before with no delay. With 0
all conflict checking will be delayed until page view. We have found
that 1 day works well in practice.

This mostly eliminates the "Merge conflict checking is in progress. Try
again in few moments." message when conflict checking is slow for big
repositories and many pull requests.

PRs that are actively being worked on will be checked immediately, while
for others are likely to actually get checked in the next few seconds.

Even better would be to auto-update the page as GitHub does, but this
relatively simple change effectively solved the problem for us.
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 12, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Sep 12, 2024
@brechtvl
Copy link
Contributor Author

This PR is rather outdated, I've updated it now to a more recent version:

  • Fixes to make it work with Gitea 1.22 and later.
  • Properly handle Gitea restarting.
  • Make pull request API access trigger checking (matching GitHub).

@lunny
Copy link
Member

lunny commented Sep 28, 2024 •
edited
Loading

I think the pull request view UI should be refactored to connect with websocket to detect whether the check status changed.

@wxiaoguang wxiaoguang self-assigned this Apr 24, 2025
@wxiaoguang
Merge branch 'main' into delay-pr-check
7ee1d02 
# Conflicts:
#	routers/web/repo/issue.go
@wxiaoguang
Copy link
Contributor

Resolved the conflict (only one in routers/web/repo/issue.go)

Will try to add some tests and improve UI

@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Apr 24, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 24, 2025 •
edited
Loading

Made some changes in 1c00cc3 (first step)

  1. Use new names, for example: StartPullRequestCheckImmediately, StartPullRequestCheckDelayable, StartPullRequestCheckOnView, checkPullRequestMergeable, testPullRequestBranchMergeable, I think they are clearer and more readable than before
  2. If the "delay check" is enabled, skip the "checking" PRs in "InitializePullRequests", I think there should be no difference to end users in most cases, and skipping behavior is more friendly to the system.
  3. Add some comments for the design and TODO
  4. Some related changes (for example: incorrect "err" usages)

@wxiaoguang wxiaoguang marked this pull request as draft April 24, 2025 13:45
@wxiaoguang
Copy link
Contributor

I think we could have the "auto refresh" UI support now: 7062121

@wxiaoguang wxiaoguang marked this pull request as ready for review April 24, 2025 15:29
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Apr 24, 2025
@wxiaoguang
Copy link
Contributor

Almost done from my side, does the new code look good to you?

@wxiaoguang wxiaoguang removed their assignment Apr 24, 2025
@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 Apr 24, 2025
brechtvl
brechtvl commented Apr 24, 2025
View reviewed changes
Copy link
Contributor Author

@brechtvl brechtvl left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It works great in testing. The code looks good to me, though the typescript part I'm not familiar enough with to properly review.

Maybe this text can be changed:

- pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments."
+ pulls.is_checking = "Checking for merge conflicts..."

Or perhaps an is-loading animation can be used instead of the ellipsis.

I updated the PR description also.

custom/conf/app.example.ini
;;
;; Delay mergeable check until page view or API access, for pull requests that have not been updated in the specified days when their base branches get updated.
;; Use "-1" to always check all pull requests (old behavior). Use "0" to always delay the checks.
;DELAY_CHECK_FOR_INACTIVE_DAYS = 7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a good default to me.

Would this be considered an API breaking change? I guess you already have to poll in case it happens to be checking, and an API access will trigger the check to happen so it should eventually return an actual status.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe we could mention it in the changelog, and collect user feedbacks.

wxiaoguang and others added 2 commits April 25, 2025 01:40
@wxiaoguang @brechtvl
Update services/pull/check.go
384a28e 
Co-authored-by: Brecht Van Lommel <[email protected]>
@wxiaoguang @brechtvl
Update services/pull/check.go
d545e22 
Co-authored-by: Brecht Van Lommel <[email protected]>
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 24, 2025 •
edited
Loading

Maybe this text can be changed:
Or perhaps an is-loading animation can be used instead of the ellipsis.

Good idea, while I think we could leave them to the future .... the "text change" would involve more tests to be updated, the "is-loading" is difficult to do quick UI tests (need to manually confirm and fine-tune .....). And this PR is large enough 😄

@wxiaoguang
Copy link
Contributor

Managed to add an animation (is-loading can't be used due to some problems), and updated the text (hopefully no hard-coded test cases would be affected)

image

@wxiaoguang wxiaoguang enabled auto-merge (squash) April 24, 2025 18:07
@wxiaoguang wxiaoguang disabled auto-merge April 24, 2025 18:12
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 24, 2025 18:23
@wxiaoguang wxiaoguang merged commit a934389 into go-gitea:main Apr 24, 2025
26 checks passed
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 25, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
c6cde9d 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Option to delay conflict checking of old pull requests until page view (go-gitea#27779)
  Update unrs-resolver (go-gitea#34279)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
docs-update-needed The document needs to be updated synchronously 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/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
5 participants
@brechtvl @lunny @wxiaoguang @techknowlogick @GiteaBot