Skip to content

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

Merged
lunny merged 23 commits into
go-gitea:mainfrom
lunny:lunny/transaction_merge
May 7, 2024
Merged

Move database operations of merging a pull request to post receive hook and add a transaction#30805
lunny merged 23 commits into
go-gitea:mainfrom
lunny:lunny/transaction_merge

Conversation

@lunny
Copy link
Copy Markdown
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 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
Comment thread routers/private/hook_post_receive.go Outdated
Comment thread modules/repository/env.go Outdated
Comment thread services/pull/merge.go Outdated
@wxiaoguang
Copy link
Copy Markdown
Contributor

Any test code?

Comment thread modules/repository/env.go Outdated
@lunny
Copy link
Copy Markdown
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
Copy Markdown
Contributor

I am sure handlePullRequestMerging should and could be tested.

@lunny
Copy link
Copy Markdown
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
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 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants