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

PullService lock via pullID #19520

Merged
merged 6 commits into from
May 4, 2022
Merged

Conversation

6543
Copy link
Member

@6543 6543 commented Apr 26, 2022

part of #9307 ... but worth own pull now independent

@6543 6543 added the type/enhancement An improvement of existing functionality label Apr 26, 2022
@6543 6543 added this to the 1.17.0 milestone Apr 26, 2022
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2022
6543 added a commit to kolaente/gitea that referenced this pull request May 1, 2022
This reverts commit 6cde7c9159a5ea75a10356feb7b8c7ad4c434a9a.
@6543 6543 added the pr/wip This PR is not ready for review label May 1, 2022
@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 3, 2022
@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 4, 2022
@6543 6543 removed the pr/wip This PR is not ready for review label May 4, 2022
@6543
Copy link
Member Author

6543 commented May 4, 2022

We should really reduce to use this memory pool mechanism.

we already use it ... I'll create a dedicated issue and will work on replace the whole sync package ...

@6543 6543 merged commit f034ee6 into go-gitea:main May 4, 2022
@6543 6543 deleted the pull_service-lock-per-pull branch May 4, 2022 16:06
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 5, 2022
* giteaofficial/main:
  Call MultipartForm.RemoveAll when request finishes (go-gitea#19606)
  Remove `RequireHighlightJS` field, update plantuml example. (go-gitea#19615)
  [skip ci] Updated translations via Crowdin
  PullService lock via pullID (go-gitea#19520)
@zeripath
Copy link
Contributor

zeripath commented May 5, 2022

hmm... Isn't the correct place to lock this kind of thing the db?

@6543
Copy link
Member Author

6543 commented May 5, 2022

Please comment on #19620

lunny pushed a commit that referenced this pull request May 7, 2022
* Fix indention

Signed-off-by: kolaente <[email protected]>

* Add option to merge a pr right now without waiting for the checks to succeed

Signed-off-by: kolaente <[email protected]>

* Fix lint

Signed-off-by: kolaente <[email protected]>

* Add scheduled pr merge to tables used for testing

Signed-off-by: kolaente <[email protected]>

* Add status param to make GetPullRequestByHeadBranch reusable

Signed-off-by: kolaente <[email protected]>

* Move "Merge now" to a seperate button to make the ui clearer

Signed-off-by: kolaente <[email protected]>

* Update models/scheduled_pull_request_merge.go

Co-authored-by: 赵智超 <[email protected]>

* Update web_src/js/index.js

Co-authored-by: 赵智超 <[email protected]>

* Update web_src/js/index.js

Co-authored-by: 赵智超 <[email protected]>

* Re-add migration after merge

* Fix frontend lint

* Fix version compare

* Add vendored dependencies

* Add basic tets

* Make sure the api route is capable of scheduling PRs for merging

* Fix comparing version

* make vendor

* adopt refactor

* apply suggestion: User -> Doer

* init var once

* Fix Test

* Update templates/repo/issue/view_content/comments.tmpl

* adopt

* nits

* next

* code format

* lint

* use same name schema; rm CreateUnScheduledPRToAutoMergeComment

* API: can not create schedule twice

* Add TestGetBranchNamesForSha

* nits

* new go routine for each pull to merge

* Update models/pull.go

Co-authored-by: a1012112796 <[email protected]>

* Update models/scheduled_pull_request_merge.go

Co-authored-by: a1012112796 <[email protected]>

* fix & add renaming sugestions

* Update services/automerge/pull_auto_merge.go

Co-authored-by: a1012112796 <[email protected]>

* fix conflict relicts

* apply latest refactors

* fix: migration after merge

* Update models/error.go

Co-authored-by: delvh <[email protected]>

* Update options/locale/locale_en-US.ini

Co-authored-by: delvh <[email protected]>

* Update options/locale/locale_en-US.ini

Co-authored-by: delvh <[email protected]>

* adapt latest refactors

* fix test

* use more context

* skip potential edgecases

* document func usage

* GetBranchNamesForSha() -> GetRefsBySha()

* start refactoring

* ajust to new changes

* nit

* docu nit

* the great check move

* move checks for branchprotection into own package

* resolve todo now ...

* move & rename

* unexport if posible

* fix

* check if merge is allowed before merge on scheduled pull

* debugg

* wording

* improve SetDefaults & nits

* NotAllowedToMerge -> DisallowedToMerge

* fix test

* merge files

* use package "errors"

* merge files

* add string names

* other implementation for gogit

* adapt refactor

* more context for models/pull.go

* GetUserRepoPermission use context

* more ctx

* use context for loading pull head/base-repo

* more ctx

* more ctx

* models.LoadIssueCtx()

* models.LoadIssueCtx()

* Handle pull_service.Merge in one DB transaction

* add TODOs

* next

* next

* next

* more ctx

* more ctx

* Start refactoring structure of old pull code ...

* move code into new packages

* shorter names ... and finish **restructure**

* Update models/branches.go

Co-authored-by: zeripath <[email protected]>

* finish UpdateProtectBranch

* more and fix

* update datum

* template: use "svg" helper

* rename prQueue 2 prPatchCheckerQueue

* handle automerge in queue

* lock pull on git&db actions ...

* lock pull on git&db actions ...

* add TODO notes

* the regex

* transaction in tests

* GetRepositoryByIDCtx

* shorter table name and lint fix

* close transaction bevore notify

* Update models/pull.go

* next

* CheckPullMergable check all branch protections!

* Update routers/web/repo/pull.go

* CheckPullMergable check all branch protections!

* Revert "PullService lock via pullID (#19520)" (for now...)

This reverts commit 6cde7c9159a5ea75a10356feb7b8c7ad4c434a9a.

* Update services/pull/check.go

* Use for a repo action one database transaction

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: delvh <[email protected]>

* Update services/issue/status.go

Co-authored-by: delvh <[email protected]>

* Update services/issue/status.go

Co-authored-by: delvh <[email protected]>

* use db.WithTx()

* gofmt

* make pr.GetDefaultMergeMessage() context aware

* make MergePullRequestForm.SetDefaults context aware

* use db.WithTx()

* pull.SetMerged only with context

* fix deadlock in `test-sqlite\#TestAPIBranchProtection`

* dont forget templates

* db.WithTx allow to set the parentCtx

* handle db transaction in service packages but not router

* issue_service.ChangeStatus just had caused another deadlock :/
it has to do something with how notification package is handled

* if we merge a pull in one database transaktion, we get a lock, because merge infoce internal api that cant handle open db sessions to the same repo

* ajust to current master

* Apply suggestions from code review

Co-authored-by: delvh <[email protected]>

* dont open db transaction in router

* make generate-swagger

* one _success less

* wording nit

* rm

* adapt

* remove not needed test files

* rm less diff & use attr in JS

* ...

* Update services/repository/files/commit.go

Co-authored-by: wxiaoguang <[email protected]>

* ajust db schema for PullAutoMerge

* skip broken pull refs

* more context in error messages

* remove webUI part for another pull

* remove more WebUI only parts

* API: add CancleAutoMergePR

* Apply suggestions from code review

Co-authored-by: wxiaoguang <[email protected]>

* fix lint

* Apply suggestions from code review

* cancle -> cancel

Co-authored-by: delvh <[email protected]>

* change queue identifyer

* fix swagger

* prevent nil issue

* fix and dont drop error

* as per @zeripath

* Update integrations/git_test.go

Co-authored-by: delvh <[email protected]>

* Update integrations/git_test.go

Co-authored-by: delvh <[email protected]>

* more declarative integration tests (dedup code)

* use assert.False/True helper

Co-authored-by: 赵智超 <[email protected]>
Co-authored-by: 6543 <[email protected]>
Co-authored-by: delvh <[email protected]>
Co-authored-by: zeripath <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* lock pull on git&db actions ...

* add TODO notes

* rename prQueue 2 prPatchCheckerQueue

* fmt
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
…9307)

* Fix indention

Signed-off-by: kolaente <[email protected]>

* Add option to merge a pr right now without waiting for the checks to succeed

Signed-off-by: kolaente <[email protected]>

* Fix lint

Signed-off-by: kolaente <[email protected]>

* Add scheduled pr merge to tables used for testing

Signed-off-by: kolaente <[email protected]>

* Add status param to make GetPullRequestByHeadBranch reusable

Signed-off-by: kolaente <[email protected]>

* Move "Merge now" to a seperate button to make the ui clearer

Signed-off-by: kolaente <[email protected]>

* Update models/scheduled_pull_request_merge.go

Co-authored-by: 赵智超 <[email protected]>

* Update web_src/js/index.js

Co-authored-by: 赵智超 <[email protected]>

* Update web_src/js/index.js

Co-authored-by: 赵智超 <[email protected]>

* Re-add migration after merge

* Fix frontend lint

* Fix version compare

* Add vendored dependencies

* Add basic tets

* Make sure the api route is capable of scheduling PRs for merging

* Fix comparing version

* make vendor

* adopt refactor

* apply suggestion: User -> Doer

* init var once

* Fix Test

* Update templates/repo/issue/view_content/comments.tmpl

* adopt

* nits

* next

* code format

* lint

* use same name schema; rm CreateUnScheduledPRToAutoMergeComment

* API: can not create schedule twice

* Add TestGetBranchNamesForSha

* nits

* new go routine for each pull to merge

* Update models/pull.go

Co-authored-by: a1012112796 <[email protected]>

* Update models/scheduled_pull_request_merge.go

Co-authored-by: a1012112796 <[email protected]>

* fix & add renaming sugestions

* Update services/automerge/pull_auto_merge.go

Co-authored-by: a1012112796 <[email protected]>

* fix conflict relicts

* apply latest refactors

* fix: migration after merge

* Update models/error.go

Co-authored-by: delvh <[email protected]>

* Update options/locale/locale_en-US.ini

Co-authored-by: delvh <[email protected]>

* Update options/locale/locale_en-US.ini

Co-authored-by: delvh <[email protected]>

* adapt latest refactors

* fix test

* use more context

* skip potential edgecases

* document func usage

* GetBranchNamesForSha() -> GetRefsBySha()

* start refactoring

* ajust to new changes

* nit

* docu nit

* the great check move

* move checks for branchprotection into own package

* resolve todo now ...

* move & rename

* unexport if posible

* fix

* check if merge is allowed before merge on scheduled pull

* debugg

* wording

* improve SetDefaults & nits

* NotAllowedToMerge -> DisallowedToMerge

* fix test

* merge files

* use package "errors"

* merge files

* add string names

* other implementation for gogit

* adapt refactor

* more context for models/pull.go

* GetUserRepoPermission use context

* more ctx

* use context for loading pull head/base-repo

* more ctx

* more ctx

* models.LoadIssueCtx()

* models.LoadIssueCtx()

* Handle pull_service.Merge in one DB transaction

* add TODOs

* next

* next

* next

* more ctx

* more ctx

* Start refactoring structure of old pull code ...

* move code into new packages

* shorter names ... and finish **restructure**

* Update models/branches.go

Co-authored-by: zeripath <[email protected]>

* finish UpdateProtectBranch

* more and fix

* update datum

* template: use "svg" helper

* rename prQueue 2 prPatchCheckerQueue

* handle automerge in queue

* lock pull on git&db actions ...

* lock pull on git&db actions ...

* add TODO notes

* the regex

* transaction in tests

* GetRepositoryByIDCtx

* shorter table name and lint fix

* close transaction bevore notify

* Update models/pull.go

* next

* CheckPullMergable check all branch protections!

* Update routers/web/repo/pull.go

* CheckPullMergable check all branch protections!

* Revert "PullService lock via pullID (go-gitea#19520)" (for now...)

This reverts commit 6cde7c9159a5ea75a10356feb7b8c7ad4c434a9a.

* Update services/pull/check.go

* Use for a repo action one database transaction

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: delvh <[email protected]>

* Update services/issue/status.go

Co-authored-by: delvh <[email protected]>

* Update services/issue/status.go

Co-authored-by: delvh <[email protected]>

* use db.WithTx()

* gofmt

* make pr.GetDefaultMergeMessage() context aware

* make MergePullRequestForm.SetDefaults context aware

* use db.WithTx()

* pull.SetMerged only with context

* fix deadlock in `test-sqlite\#TestAPIBranchProtection`

* dont forget templates

* db.WithTx allow to set the parentCtx

* handle db transaction in service packages but not router

* issue_service.ChangeStatus just had caused another deadlock :/
it has to do something with how notification package is handled

* if we merge a pull in one database transaktion, we get a lock, because merge infoce internal api that cant handle open db sessions to the same repo

* ajust to current master

* Apply suggestions from code review

Co-authored-by: delvh <[email protected]>

* dont open db transaction in router

* make generate-swagger

* one _success less

* wording nit

* rm

* adapt

* remove not needed test files

* rm less diff & use attr in JS

* ...

* Update services/repository/files/commit.go

Co-authored-by: wxiaoguang <[email protected]>

* ajust db schema for PullAutoMerge

* skip broken pull refs

* more context in error messages

* remove webUI part for another pull

* remove more WebUI only parts

* API: add CancleAutoMergePR

* Apply suggestions from code review

Co-authored-by: wxiaoguang <[email protected]>

* fix lint

* Apply suggestions from code review

* cancle -> cancel

Co-authored-by: delvh <[email protected]>

* change queue identifyer

* fix swagger

* prevent nil issue

* fix and dont drop error

* as per @zeripath

* Update integrations/git_test.go

Co-authored-by: delvh <[email protected]>

* Update integrations/git_test.go

Co-authored-by: delvh <[email protected]>

* more declarative integration tests (dedup code)

* use assert.False/True helper

Co-authored-by: 赵智超 <[email protected]>
Co-authored-by: 6543 <[email protected]>
Co-authored-by: delvh <[email protected]>
Co-authored-by: zeripath <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants