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.

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

Drop timeout for requests made to the internal hook api #33947

Merged
merged 6 commits into from
Mar 21, 2025
Merged

Drop timeout for requests made to the internal hook api #33947

merged 6 commits into from
Mar 21, 2025

Conversation

Mik4sa
Copy link
Contributor

@Mik4sa Mik4sa commented Mar 20, 2025 •
edited
Loading

This change targets #32663
We drop the hardcoded timeout of 60 seconds for requests to the internal hook api. With this change the timeout is completly removed.

Note: I never really worked with go. Please keep that in mind when giving feedback :)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 20, 2025
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 20, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 20, 2025
@Mik4sa Mik4sa force-pushed the configurable-internal-hook-timeout branch from 8cc6812 to 2f889f2 Compare March 20, 2025 15:37
@wxiaoguang
Copy link
Contributor

Thank you for investigating the problem!

Maybe we can't use Git.Timeout.Default because it is mostly used for "web git operations" and some users might set it to a pretty small value (maybe around a minute).

Actually the Git.Timeout.Default is a legacy config option and already caused some other problems ......

@wxiaoguang
Copy link
Contributor

I think we could simply remove this "timeout" setting. No timeout should be good enough.

@Mik4sa Mik4sa force-pushed the configurable-internal-hook-timeout branch from 5b0111a to e192c3f Compare March 21, 2025 05:29
@Mik4sa Mik4sa force-pushed the configurable-internal-hook-timeout branch from e192c3f to f4061d7 Compare March 21, 2025 07:26
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 21, 2025
@Mik4sa Mik4sa changed the title Use git.timeout/DEFAULT for requests made to the internal hook api Drop timeout for requests made to the internal hook api Mar 21, 2025
@wxiaoguang
Copy link
Contributor

I will try to push some commits here 🙏

@wxiaoguang wxiaoguang self-assigned this Mar 21, 2025
@Mik4sa
Copy link
Contributor Author

Mik4sa commented Mar 21, 2025

This would be great. I actually have very limited time for that.

@wxiaoguang
Copy link
Contributor

ps: since your PR is from an github org, so maintainers can't edit or sync it with latest main branch (github's limit). Gitea's PR workflow requires all PRs must be merged with latest main branch first to pass CI. So after this PR gets approved, you still need to manually click "update branch" one or two times to help to get it merged into main.

Usually a PR from a individual's account is easier to co-work, maintainers could help to edit and merge it by themselves.

@Mik4sa
Copy link
Contributor Author

Mik4sa commented Mar 21, 2025

Oh good to know. I was wondering why I didn't found that option. Now I know. I'll keep that in mind for new PRs.
Ofcourse I'm going to keep that branch updated then :)

@Mik4sa
Copy link
Contributor Author

Mik4sa commented Mar 21, 2025

I moved the org repo to my personal repo and checked allow edits by maintainers
So you should be able to do everything as expected now :)?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 21, 2025 •
edited
Loading

Thank you very much. Let's update to Mik4sa#1 (done)

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 21, 2025
@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 Mar 21, 2025
@wxiaoguang wxiaoguang added type/bug backport/v1.23 This PR should be backported to Gitea 1.23 labels Mar 21, 2025
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Mar 21, 2025
TheFox0x7
TheFox0x7 reviewed Mar 21, 2025
View reviewed changes
@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 Mar 21, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 21, 2025
@lunny lunny merged commit b5d2e31 into go-gitea:main Mar 21, 2025
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 21, 2025
@Mik4sa @wxiaoguang @GiteaBot
Drop timeout for requests made to the internal hook api (go-gitea#33947)
545665f 
This change targets go-gitea#32663
We drop the hardcoded timeout of 60 seconds for requests to the internal
hook api. With this change the timeout is completly removed.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot mentioned this pull request Mar 21, 2025
Merged
@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 Mar 21, 2025
wxiaoguang added a commit that referenced this pull request Mar 22, 2025
@GiteaBot @Mik4sa @wxiaoguang
Drop timeout for requests made to the internal hook api (#33947) (#33970
b5f8c4a 
)

Backport #33947 by Mik4sa

Co-authored-by: Kai Leonhardt <8343141+Mik4sa@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 24, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
fde02d1 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Only use prev and next buttons for pagination on user dashboard (go-gitea#33981)
  update jwt and redis packages (go-gitea#33984)
  [skip ci] Updated translations via Crowdin
  Improve oauth2 error handling (go-gitea#33969)
  [skip ci] Updated translations via Crowdin
  Cover `go.mod` and `go.sum` in `.editorconfig` (go-gitea#33960)
  Drop timeout for requests made to the internal hook api (go-gitea#33947)
  Fix file name could not be searched if the file was not a text file when using the Bleve indexer (go-gitea#33959)
  Fix oauth2 auth and UI (go-gitea#33961)
  Allow filtering issues by any assignee (go-gitea#33343)
  Optimize total count of feed when loading activities in user dashboard. (go-gitea#33841)
  Extract code to their own functions for push update (go-gitea#33944)
  Optimize heatmap query (go-gitea#33853)
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

@TheFox0x7 TheFox0x7 TheFox0x7 left review comments

@lunny lunny lunny approved these changes

Assignees

@wxiaoguang wxiaoguang

Labels
backport/done All backports for this PR have been created backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.24.0
Development

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