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

Move database operations of merging a pull request to post receive hook and add a transaction #30805

Merged
merged 23 commits into from
May 7, 2024

Conversation

lunny
Copy link
Member

@lunny lunny commented May 1, 2024

Merging PR may fail because of various problems. The pull request may have a dirty state because there is no transaction when merging a pull request. ref #25741 (comment)

This PR moves all database update operations to post-receive handler for merging a pull request and having a database transaction. That means if database operations fail, then the git merging will fail, the git client will get a fail result.

There are already many tests for pull request merging, so we don't need to add a new one.

@lunny lunny added type/bug backport/v1.22 This PR should be backported to Gitea 1.22 labels May 1, 2024
@lunny lunny added this to the 1.23.0 milestone May 1, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 1, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 1, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels May 1, 2024
services/pull/merge.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

Any test code?

@lunny
Copy link
Member Author

lunny commented May 3, 2024

Any test code?

There are already enough tests for merging a pull request. And it's difficult to do a mock of updating database failure.

@wxiaoguang
Copy link
Contributor

I am sure handlePullRequestMerging should and could be tested.

@lunny
Copy link
Member Author

lunny commented May 7, 2024

@wxiaoguang Tests for auto-merge deletion added. 03ab2fa

@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 May 7, 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 May 7, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 7, 2024
@lunny lunny enabled auto-merge (squash) May 7, 2024 07:09
@lunny lunny merged commit ebf0c96 into go-gitea:main May 7, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request May 7, 2024
…ok and add a transaction (go-gitea#30805)

Merging PR may fail because of various problems. The pull request may
have a dirty state because there is no transaction when merging a pull
request. ref
go-gitea#25741 (comment)

This PR moves all database update operations to post-receive handler for
merging a pull request and having a database transaction. That means if
database operations fail, then the git merging will fail, the git client
will get a fail result.

There are already many tests for pull request merging, so we don't need
to add a new one.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@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 May 7, 2024
@lunny lunny deleted the lunny/transaction_merge branch May 7, 2024 07:42
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 8, 2024
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Apply to become a maintainer (go-gitea#30884)
  Refactor AppURL usage (go-gitea#30885)
  Move database operations of merging a pull request to post receive hook and add a transaction (go-gitea#30805)
  Fix missing migrate actions artifacts (go-gitea#30874)
lunny added a commit that referenced this pull request May 8, 2024
…ok and add a transaction (#30805) (#30888)

Backport #30805 by @lunny

Merging PR may fail because of various problems. The pull request may
have a dirty state because there is no transaction when merging a pull
request. ref
#25741 (comment)

This PR moves all database update operations to post-receive handler for
merging a pull request and having a database transaction. That means if
database operations fail, then the git merging will fail, the git client
will get a fail result.

There are already many tests for pull request merging, so we don't need
to add a new one.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
lunny added a commit that referenced this pull request May 21, 2024
…ered (#30780)

Replace #25741
Close #24445
Close #30658
Close #20646
~Depends on #30805~

Since #25741 has been rewritten totally, to make the contribution
easier, I will continue the work in this PR. Thanks @6543

---------

Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request May 21, 2024
…ered (go-gitea#30780)

Replace go-gitea#25741
Close go-gitea#24445
Close go-gitea#30658
Close go-gitea#20646
~Depends on go-gitea#30805~

Since go-gitea#25741 has been rewritten totally, to make the contribution
easier, I will continue the work in this PR. Thanks @6543

---------

Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
lunny added a commit that referenced this pull request May 22, 2024
…ered (#30780) (#31039)

Backport #30780 by @lunny

Replace #25741
Close #24445
Close #30658
Close #20646
~Depends on #30805~

Since #25741 has been rewritten totally, to make the contribution
easier, I will continue the work in this PR. Thanks @6543

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 5, 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.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants