diff --git a/models/fixtures/pull_auto_merge.yml b/models/fixtures/pull_auto_merge.yml new file mode 100644 index 0000000000..ca780a73aa --- /dev/null +++ b/models/fixtures/pull_auto_merge.yml @@ -0,0 +1 @@ +[] # empty diff --git a/models/pull/automerge.go b/models/pull/automerge.go index 63f572309b..dcc1f39271 100644 --- a/models/pull/automerge.go +++ b/models/pull/automerge.go @@ -10,6 +10,7 @@ import ( "forgejo.org/models/db" repo_model "forgejo.org/models/repo" user_model "forgejo.org/models/user" + "forgejo.org/modules/log" "forgejo.org/modules/timeutil" ) @@ -58,13 +59,15 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, return ErrAlreadyScheduledToAutoMerge{PullID: pullID} } - _, err := db.GetEngine(ctx).Insert(&AutoMerge{ + scheduledPRM, err := db.GetEngine(ctx).Insert(&AutoMerge{ DoerID: doer.ID, PullID: pullID, MergeStyle: style, Message: message, DeleteBranchAfterMerge: deleteBranch, }) + log.Trace("ScheduleAutoMerge %+v for PR %d", scheduledPRM, pullID) + return err } @@ -81,6 +84,8 @@ func GetScheduledMergeByPullID(ctx context.Context, pullID int64) (bool, *AutoMe return false, nil, err } + log.Trace("GetScheduledMergeByPullID found %+v for PR %d", scheduledPRM, pullID) + scheduledPRM.Doer = doer return true, scheduledPRM, nil } @@ -94,6 +99,8 @@ func DeleteScheduledAutoMerge(ctx context.Context, pullID int64) error { return db.ErrNotExist{Resource: "auto_merge", ID: pullID} } + log.Trace("DeleteScheduledAutoMerge %+v for PR %d", scheduledPRM, pullID) + _, err = db.GetEngine(ctx).ID(scheduledPRM.ID).Delete(&AutoMerge{}) return err } diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index f183136907..cbfe3bd54e 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -107,6 +107,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } if !exists { + log.Trace("GetScheduledMergeByPullID found nothing for PR %d", pullID) return } @@ -204,6 +205,10 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + log.Error("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) + } + if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) // FIXME: if merge failed, we should display some error message to the pull request page. diff --git a/services/pull/check.go b/services/pull/check.go index afeb7e675e..6002e2ae26 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -28,6 +28,7 @@ import ( "forgejo.org/modules/timeutil" asymkey_service "forgejo.org/services/asymkey" notify_service "forgejo.org/services/notify" + shared_automerge "forgejo.org/services/shared/automerge" ) // prPatchCheckerQueue represents a queue to handle update pull request tests @@ -170,7 +171,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer // checkAndUpdateStatus checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. -func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { +func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) bool { // If status has not been changed to conflict by testPatch then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable @@ -184,12 +185,15 @@ func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { if has { log.Trace("Not updating status for %-v as it is due to be rechecked", pr) - return + return false } if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { log.Error("Update[%-v]: %v", pr, err) + return false } + + return true } // getMergeCommit checks if a pull request has been merged @@ -339,15 +343,22 @@ func handler(items ...string) []string { } func testPR(id int64) { - pullWorkingPool.CheckIn(fmt.Sprint(id)) - defer pullWorkingPool.CheckOut(fmt.Sprint(id)) ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("Test PR[%d] from patch checking queue", id)) defer finished() + if pr, updated := testPRProtected(ctx, id); pr != nil && updated { + shared_automerge.AddToQueueIfMergeable(ctx, pr) + } +} + +func testPRProtected(ctx context.Context, id int64) (*issues_model.PullRequest, bool) { + pullWorkingPool.CheckIn(fmt.Sprint(id)) + defer pullWorkingPool.CheckOut(fmt.Sprint(id)) + pr, err := issues_model.GetPullRequestByID(ctx, id) if err != nil { log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err) - return + return nil, false } log.Trace("Testing %-v", pr) @@ -357,12 +368,12 @@ func testPR(id int64) { if pr.HasMerged { log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return + return nil, false } if manuallyMerged(ctx, pr) { log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) - return + return nil, false } if err := TestPatch(pr); err != nil { @@ -371,9 +382,10 @@ func testPR(id int64) { if err := pr.UpdateCols(ctx, "status"); err != nil { log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) } - return + return nil, false } - checkAndUpdateStatus(ctx, pr) + + return pr, checkAndUpdateStatus(ctx, pr) } // CheckPRsForBaseBranch check all pulls with baseBrannch diff --git a/services/shared/automerge/automerge.go b/services/shared/automerge/automerge.go index 1dc309f4b3..be7b2f6eb4 100644 --- a/services/shared/automerge/automerge.go +++ b/services/shared/automerge/automerge.go @@ -21,9 +21,9 @@ import ( var PRAutoMergeQueue *queue.WorkerPoolQueue[string] func addToQueue(pr *issues_model.PullRequest, sha string) { - log.Trace("Adding pullID: %d to the pull requests patch checking queue with sha %s", pr.ID, sha) + log.Trace("Adding pullID: %d to the automerge queue with sha %s", pr.ID, sha) if err := PRAutoMergeQueue.Push(fmt.Sprintf("%d_%s", pr.ID, sha)); err != nil { - log.Error("Error adding pullID: %d to the pull requests patch checking queue %v", pr.ID, err) + log.Error("Error adding pullID: %d to the automerge queue %v", pr.ID, err) } } @@ -43,32 +43,29 @@ func StartPRCheckAndAutoMergeBySHA(ctx context.Context, sha string, repo *repo_m return nil } -// StartPRCheckAndAutoMerge start an automerge check and auto merge task for a pull request func StartPRCheckAndAutoMerge(ctx context.Context, pull *issues_model.PullRequest) { if pull == nil || pull.HasMerged || !pull.CanAutoMerge() { return } - if err := pull.LoadBaseRepo(ctx); err != nil { - log.Error("LoadBaseRepo: %v", err) - return + commitID := pull.HeadCommitID + if commitID == "" { + commitID = getCommitIDFromRefName(ctx, pull) } - gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) - if err != nil { - log.Error("OpenRepository: %v", err) - return - } - defer gitRepo.Close() - commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) - if err != nil { - log.Error("GetRefCommitID: %v", err) + if commitID == "" { return } addToQueue(pull, commitID) } +var AddToQueueIfMergeable = func(ctx context.Context, pull *issues_model.PullRequest) { + if pull.Status == issues_model.PullRequestStatusMergeable { + StartPRCheckAndAutoMerge(ctx, pull) + } +} + func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model.Repository, filter func(*issues_model.PullRequest) bool) (map[int64]*issues_model.PullRequest, error) { gitRepo, err := gitrepo.OpenRepository(ctx, repo) if err != nil { @@ -118,3 +115,24 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. return pulls, nil } + +func getCommitIDFromRefName(ctx context.Context, pull *issues_model.PullRequest) string { + if err := pull.LoadBaseRepo(ctx); err != nil { + log.Error("LoadBaseRepo: %v", err) + return "" + } + + gitRepo, err := gitrepo.OpenRepository(ctx, pull.BaseRepo) + if err != nil { + log.Error("OpenRepository: %v", err) + return "" + } + defer gitRepo.Close() + commitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName()) + if err != nil { + log.Error("GetRefCommitID: %v", err) + return "" + } + + return commitID +} diff --git a/tests/integration/README.md b/tests/integration/README.md index d83685388e..a6be7fe72c 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -71,11 +71,11 @@ TEST_MYSQL_HOST=localhost:3306 TEST_MYSQL_DBNAME=test?multiStatements=true TEST_ ### Run pgsql integration tests Setup a pgsql database inside docker ``` -docker run -e "POSTGRES_DB=test" -e POSTGRES_PASSWORD=postgres -p 5432:5432 --rm --name pgsql postgres:latest #(Ctrl-c to stop the database) +docker run -e "POSTGRES_DB=test" -e POSTGRES_PASSWORD=postgres POSTGRESQL_FSYNC=off POSTGRESQL_EXTRA_FLAGS="-c full_page_writes=off" -p 5432:5432 --rm --name pgsql data.forgejo.org/oci/bitnami/postgresql:16 #(Ctrl-c to stop the database) ``` Start tests based on the database container ``` -TEST_STORAGE_TYPE=local TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres make test-pgsql +TEST_STORAGE_TYPE=local TEST_PGSQL_HOST=localhost:5432 TEST_PGSQL_DBNAME=test TEST_PGSQL_USERNAME=postgres TEST_PGSQL_PASSWORD=postgres make 'test-pgsql#Test' ``` ### Running individual tests diff --git a/tests/integration/patch_status_test.go b/tests/integration/patch_status_test.go index 078051fe63..3ce1dc4cb9 100644 --- a/tests/integration/patch_status_test.go +++ b/tests/integration/patch_status_test.go @@ -4,6 +4,7 @@ package integration import ( + "context" "fmt" "net/http" "net/url" @@ -20,7 +21,10 @@ import ( user_model "forgejo.org/models/user" "forgejo.org/modules/git" "forgejo.org/modules/optional" + "forgejo.org/modules/test" + pull_service "forgejo.org/services/pull" files_service "forgejo.org/services/repository/files" + shared_automerge "forgejo.org/services/shared/automerge" "forgejo.org/tests" "github.com/stretchr/testify/assert" @@ -51,6 +55,20 @@ func TestPatchStatus(t *testing.T) { }) defer f() + testAutomergeQueued := func(t *testing.T, pr *issues_model.PullRequest, expected issues_model.PullRequestStatus) { + t.Helper() + + var actual issues_model.PullRequestStatus = -1 + defer test.MockVariableValue(&shared_automerge.AddToQueueIfMergeable, func(ctx context.Context, pull *issues_model.PullRequest) { + actual = pull.Status + })() + + pull_service.AddToTaskQueue(t.Context(), pr) + assert.Eventually(t, func() bool { + return expected == actual + }, time.Second*5, time.Millisecond*200) + } + testRepoFork(t, session, "user2", repo.Name, "org3", "forked-repo") forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "org3", Name: "forked-repo"}) @@ -91,7 +109,9 @@ func TestPatchStatus(t *testing.T) { require.NoError(t, git.NewCommand(t.Context(), "push", "fork", "HEAD:normal").Run(&git.RunOpts{Dir: dstPath})) testPullCreateDirectly(t, session, repo.OwnerName, repo.Name, repo.DefaultBranch, forkRepo.OwnerName, forkRepo.Name, "normal", "across repo normal") - test(t, unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0")) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0") + test(t, pr) + testAutomergeQueued(t, pr, issues_model.PullRequestStatusMergeable) }) t.Run("Same repository", func(t *testing.T) { @@ -144,7 +164,9 @@ func TestPatchStatus(t *testing.T) { t.Run("Existing", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - test(t, unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0")) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkRepo.ID, HeadBranch: "normal"}, "flow = 0") + test(t, pr) + testAutomergeQueued(t, pr, issues_model.PullRequestStatusConflict) }) t.Run("New", func(t *testing.T) {