From f0dd070ef1650daa30ab090d7d6117d71242c6ed Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 27 Feb 2025 11:44:06 -0800 Subject: [PATCH 01/10] pull request updates will also trigger code owners review requests --- services/issue/issue.go | 6 +++++- services/issue/pull.go | 11 ++++++----- services/pull/pull.go | 11 ++++++++++- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/services/issue/issue.go b/services/issue/issue.go index 586b6031c8460..455a1ec29781b 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -92,8 +92,12 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode var reviewNotifiers []*ReviewRequestNotifier if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { + if err := issue.LoadPullRequest(ctx); err != nil { + return err + } + var err error - reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest) + reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue.PullRequest) if err != nil { log.Error("PullRequestCodeOwnersReview: %v", err) } diff --git a/services/issue/pull.go b/services/issue/pull.go index 896802108d6ca..3f20e2da825b7 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -40,17 +40,17 @@ type ReviewRequestNotifier struct { ReviewTeam *org_model.Team } -func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { - files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - +func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { + if err := pr.LoadIssue(ctx); err != nil { + return nil, err + } + issue := pr.Issue if pr.IsWorkInProgress(ctx) { return nil, nil } - if err := pr.LoadHeadRepo(ctx); err != nil { return nil, err } - if err := pr.LoadBaseRepo(ctx); err != nil { return nil, err } @@ -71,6 +71,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, } var data string + files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} for _, file := range files { if blob, err := commit.GetBlobByPath(file); err == nil { data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) diff --git a/services/pull/pull.go b/services/pull/pull.go index 5d3758eca6d14..fd742ebe6e1c7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -176,7 +176,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, issue, pr) + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) if err != nil { return err } @@ -453,6 +453,15 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } } + if !pr.IsWorkInProgress(ctx) { + reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(ctx, pr) + if err != nil { + log.Error("PullRequestCodeOwnersReview: %v", err) + } else { + issue_service.ReviewRequestNotify(ctx, pr.Issue, doer, reviewNotifiers) + } + } + notify_service.PullRequestSynchronized(ctx, doer, pr) } } From 172178c36716e4648f6774d6f25e1f5116464ec5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 28 Feb 2025 13:58:33 -0800 Subject: [PATCH 02/10] merge the code owner file list --- routers/web/repo/view_file.go | 3 +-- services/issue/pull.go | 11 +++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/routers/web/repo/view_file.go b/routers/web/repo/view_file.go index 5784297918436..4ce7a8e3a4480 100644 --- a/routers/web/repo/view_file.go +++ b/routers/web/repo/view_file.go @@ -9,7 +9,6 @@ import ( "image" "io" "path" - "slices" "strings" git_model "code.gitea.io/gitea/models/git" @@ -79,7 +78,7 @@ func prepareToRenderFile(ctx *context.Context, entry *git.TreeEntry) { if workFlowErr != nil { ctx.Data["FileError"] = ctx.Locale.Tr("actions.runs.invalid_workflow_helper", workFlowErr.Error()) } - } else if slices.Contains([]string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}, ctx.Repo.TreePath) { + } else if issue_service.IsCodeOwnerFile(ctx.Repo.TreePath) { if data, err := blob.GetBlobContent(setting.UI.MaxDisplayFileSize); err == nil { _, warnings := issue_model.GetCodeOwnersFromContent(ctx, data) if len(warnings) > 0 { diff --git a/services/issue/pull.go b/services/issue/pull.go index 3f20e2da825b7..0b23fe8a26db5 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -6,6 +6,7 @@ package issue import ( "context" "fmt" + "slices" "time" issues_model "code.gitea.io/gitea/models/issues" @@ -40,6 +41,12 @@ type ReviewRequestNotifier struct { ReviewTeam *org_model.Team } +var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} + +func IsCodeOwnerFile(f string) bool { + return slices.Contains(codeOwnerFiles, f) +} + func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { if err := pr.LoadIssue(ctx); err != nil { return nil, err @@ -71,8 +78,8 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque } var data string - files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - for _, file := range files { + + for _, file := range codeOwnerFiles { if blob, err := commit.GetBlobByPath(file); err == nil { data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) if err == nil { From b31d067d72f12cb625a06e9761b896881464d96c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 1 Mar 2025 21:28:25 -0800 Subject: [PATCH 03/10] Only request code owner when syncing commits files ownered by the code owners. --- services/issue/pull.go | 25 +++++++++++++++++++------ services/pull/merge.go | 2 +- services/pull/pull.go | 12 +++++++++--- services/pull/update.go | 4 ++-- services/repository/push.go | 4 +++- 5 files changed, 34 insertions(+), 13 deletions(-) diff --git a/services/issue/pull.go b/services/issue/pull.go index 0b23fe8a26db5..8d751f54989a7 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -48,6 +48,10 @@ func IsCodeOwnerFile(f string) bool { } func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { + return PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, "", "") +} + +func PullRequestCodeOwnersReviewSpecialCommits(ctx context.Context, pr *issues_model.PullRequest, startCommitID, endCommitID string) ([]*ReviewRequestNotifier, error) { if err := pr.LoadIssue(ctx); err != nil { return nil, err } @@ -78,7 +82,6 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque } var data string - for _, file := range codeOwnerFiles { if blob, err := commit.GetBlobByPath(file); err == nil { data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) @@ -87,18 +90,28 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque } } } + if data == "" { + return nil, nil + } rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) + if len(rules) == 0 { + return nil, nil + } - // get the mergebase - mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) - if err != nil { - return nil, err + if startCommitID == "" && endCommitID == "" { + // get the mergebase + mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) + if err != nil { + return nil, err + } + startCommitID = mergeBase + endCommitID = pr.GetGitRefName() } // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed // between the merge base and the head commit but not the base branch and the head commit - changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitRefName()) + changedFiles, err := repo.GetFilesChangedBetween(startCommitID, endCommitID) if err != nil { return nil, err } diff --git a/services/pull/merge.go b/services/pull/merge.go index 9c909ef7958b1..68a3f433e5e42 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -211,7 +211,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } defer releaser() defer func() { - go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") + go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, false, "", "") }() _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) diff --git a/services/pull/pull.go b/services/pull/pull.go index fd742ebe6e1c7..7ca5ee545d6c1 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -374,7 +374,7 @@ func checkForInvalidation(ctx context.Context, requests issues_model.PullRequest // AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch, // and generate new patch for testing as needed. -func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) { +func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, isSync, isForPush bool, oldCommitID, newCommitID string) { log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch) graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { // There is no sensible way to shut this down ":-(" @@ -454,10 +454,16 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(ctx, pr) + var reviewNotifiers []*issue_service.ReviewRequestNotifier + if isForPush { + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) + } else { + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, oldCommitID, newCommitID) + } if err != nil { log.Error("PullRequestCodeOwnersReview: %v", err) - } else { + } + if len(reviewNotifiers) > 0 { issue_service.ReviewRequestNotify(ctx, pr.Issue, doer, reviewNotifiers) } } diff --git a/services/pull/update.go b/services/pull/update.go index abf7ad45091ae..5708d2dfbfd41 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -42,7 +42,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. if rebase { defer func() { - go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") + go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, false, "", "") }() return updateHeadByRebaseOnToBase(ctx, pr, doer) @@ -83,7 +83,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase) defer func() { - go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "") + go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, false, "", "") }() return err diff --git a/services/repository/push.go b/services/repository/push.go index 0ea51f9c072d2..7be2f06bc97a5 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -166,7 +166,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { branch := opts.RefFullName.BranchName() if !opts.IsDelRef() { log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) newCommit, err := gitRepo.GetCommit(opts.NewCommitID) if err != nil { @@ -208,6 +207,9 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { log.Error("IsForcePush %s:%s failed: %v", repo.FullName(), branch, err) } + // only update branch can trigger pull request task because the pull request hasn't been created yet when creaing a branch + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, isForcePush, opts.OldCommitID, opts.NewCommitID) + if isForcePush { log.Trace("Push %s is a force push", opts.NewCommitID) From dd1eb5249748ffdbc20700011b9a5e56a5b1ee1d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 7 Mar 2025 15:33:55 -0800 Subject: [PATCH 04/10] Use a structure as parameters for TestPullrequestTask --- services/pull/merge.go | 10 +++++++- services/pull/pull.go | 48 ++++++++++++++++++++++--------------- services/pull/update.go | 20 ++++++++++++++-- services/repository/push.go | 10 +++++++- 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 68a3f433e5e42..0acbfe22c31e1 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -211,7 +211,15 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } defer releaser() defer func() { - go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, false, "", "") + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) }() _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) diff --git a/services/pull/pull.go b/services/pull/pull.go index 7ca5ee545d6c1..7b8667238d330 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -372,19 +372,29 @@ func checkForInvalidation(ctx context.Context, requests issues_model.PullRequest return nil } +type TestPullRequestOptions struct { + RepoID int64 + Doer *user_model.User + Branch string + IsSync bool + IsForcePush bool + OldCommitID string + NewCommitID string +} + // AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch, // and generate new patch for testing as needed. -func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, isSync, isForPush bool, oldCommitID, newCommitID string) { - log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch) +func AddTestPullRequestTask(opts TestPullRequestOptions) { + log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { // There is no sensible way to shut this down ":-(" // If you don't let it run all the way then you will lose data // TODO: graceful: AddTestPullRequestTask needs to become a queue! // GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR. - prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch) if err != nil { - log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err) + log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } @@ -400,25 +410,25 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } AddToTaskQueue(ctx, pr) - comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) + comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID) if err == nil && comment != nil { - notify_service.PullRequestPushCommits(ctx, doer, pr, comment) + notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment) } } - if isSync { + if opts.IsSync { requests := issues_model.PullRequestList(prs) if err = requests.LoadAttributes(ctx); err != nil { log.Error("PullRequestList.LoadAttributes: %v", err) } - if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch); invalidationErr != nil { + if invalidationErr := checkForInvalidation(ctx, requests, opts.RepoID, opts.Doer, opts.Branch); invalidationErr != nil { log.Error("checkForInvalidation: %v", invalidationErr) } if err == nil { for _, pr := range prs { objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) - if newCommitID != "" && newCommitID != objectFormat.EmptyObjectID().String() { - changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID) + if opts.NewCommitID != "" && opts.NewCommitID != objectFormat.EmptyObjectID().String() { + changed, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID) if err != nil { log.Error("checkIfPRContentChanged: %v", err) } @@ -434,12 +444,12 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, log.Error("GetFirstMatchProtectedBranchRule: %v", err) } if pb != nil && pb.DismissStaleApprovals { - if err := DismissApprovalReviews(ctx, doer, pr); err != nil { + if err := DismissApprovalReviews(ctx, opts.Doer, pr); err != nil { log.Error("DismissApprovalReviews: %v", err) } } } - if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, newCommitID); err != nil { + if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil { log.Error("MarkReviewsAsNotStale: %v", err) } divergence, err := GetDiverging(ctx, pr) @@ -455,28 +465,28 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, if !pr.IsWorkInProgress(ctx) { var reviewNotifiers []*issue_service.ReviewRequestNotifier - if isForPush { + if opts.IsForcePush { reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) } else { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, oldCommitID, newCommitID) + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, opts.OldCommitID, opts.NewCommitID) } if err != nil { log.Error("PullRequestCodeOwnersReview: %v", err) } if len(reviewNotifiers) > 0 { - issue_service.ReviewRequestNotify(ctx, pr.Issue, doer, reviewNotifiers) + issue_service.ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) } } - notify_service.PullRequestSynchronized(ctx, doer, pr) + notify_service.PullRequestSynchronized(ctx, opts.Doer, pr) } } } - log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch) - prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) + log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) + prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch) if err != nil { - log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repoID, branch, err) + log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } for _, pr := range prs { diff --git a/services/pull/update.go b/services/pull/update.go index 5708d2dfbfd41..3e00dd4e65f72 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -42,7 +42,15 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. if rebase { defer func() { - go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, false, "", "") + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) }() return updateHeadByRebaseOnToBase(ctx, pr, doer) @@ -83,7 +91,15 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase) defer func() { - go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, false, "", "") + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: reversePR.HeadRepo.ID, + Doer: doer, + Branch: reversePR.HeadBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) }() return err diff --git a/services/repository/push.go b/services/repository/push.go index 7be2f06bc97a5..00d4fb11afeed 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -208,7 +208,15 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } // only update branch can trigger pull request task because the pull request hasn't been created yet when creaing a branch - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, isForcePush, opts.OldCommitID, opts.NewCommitID) + go pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{ + RepoID: repo.ID, + Doer: pusher, + Branch: branch, + IsSync: true, + IsForcePush: isForcePush, + OldCommitID: opts.OldCommitID, + NewCommitID: opts.NewCommitID, + }) if isForcePush { log.Trace("Push %s is a force push", opts.NewCommitID) From 449ed5b125e6a184d7e5c05672c9eb06d46d8652 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 11 Mar 2025 11:31:25 -0700 Subject: [PATCH 05/10] Add test for PullRequestCodeOwnersReview and PullRequestCodeOwnersReviewSpecialCommits --- services/issue/pull.go | 1 + tests/integration/pull_review_test.go | 40 +++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/services/issue/pull.go b/services/issue/pull.go index 8d751f54989a7..1261a5c7ca451 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -65,6 +65,7 @@ func PullRequestCodeOwnersReviewSpecialCommits(ctx context.Context, pr *issues_m if err := pr.LoadBaseRepo(ctx); err != nil { return nil, err } + pr.Issue.Repo = pr.BaseRepo if pr.BaseRepo.IsFork { return nil, nil diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 68de421413693..96cab3fee65a2 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -67,7 +67,7 @@ func TestPullView_CodeOwner(t *testing.T) { { Operation: "create", TreePath: "CODEOWNERS", - ContentReader: strings.NewReader("README.md @user5\n"), + ContentReader: strings.NewReader("README.md @user5\nuser8-file.md @user8\n"), }, }, }) @@ -75,7 +75,7 @@ func TestPullView_CodeOwner(t *testing.T) { t.Run("First Pull Request", func(t *testing.T) { // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + resp1, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch", Files: []*files_service.ChangeRepoFile{ { @@ -95,7 +95,36 @@ func TestPullView_CodeOwner(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) assert.NoError(t, pr.LoadIssue(db.DefaultContext)) - err := issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request") + reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr) + assert.NoError(t, err) + assert.Len(t, reviewNotifiers, 1) + assert.EqualValues(t, 5, reviewNotifiers[0].Reviewer.ID) + + // update the file on the pr branch + resp2, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + OldBranch: "codeowner-basebranch", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "user8-file.md", + ContentReader: strings.NewReader("# This is a new project2\n"), + }, + }, + }) + assert.NoError(t, err) + + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr) + assert.NoError(t, err) + assert.Len(t, reviewNotifiers, 2) + assert.EqualValues(t, 5, reviewNotifiers[0].Reviewer.ID) + assert.EqualValues(t, 8, reviewNotifiers[1].Reviewer.ID) + + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(db.DefaultContext, pr, resp1.Commit.SHA, resp2.Commit.SHA) + assert.NoError(t, err) + assert.Len(t, reviewNotifiers, 1) + assert.EqualValues(t, 8, reviewNotifiers[0].Reviewer.ID) + + err = issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request") assert.NoError(t, err) prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.NoError(t, prUpdated1.LoadIssue(db.DefaultContext)) @@ -140,6 +169,11 @@ func TestPullView_CodeOwner(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"}) unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + + reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr) + assert.NoError(t, err) + assert.Len(t, reviewNotifiers, 1) + assert.EqualValues(t, 8, reviewNotifiers[0].Reviewer.ID) }) t.Run("Forked Repo Pull Request", func(t *testing.T) { From 6f792673bb65dc9cb96bfdbeb3d2c2e48be4ffa8 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 12 Mar 2025 19:14:33 +0800 Subject: [PATCH 06/10] Update services/issue/pull.go --- services/issue/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/issue/pull.go b/services/issue/pull.go index 1261a5c7ca451..bd19c254362b9 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -48,7 +48,7 @@ func IsCodeOwnerFile(f string) bool { } func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { - return PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, "", "") + return PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, "", "") // no commit is provided, then it uses PR's base&head branch } func PullRequestCodeOwnersReviewSpecialCommits(ctx context.Context, pr *issues_model.PullRequest, startCommitID, endCommitID string) ([]*ReviewRequestNotifier, error) { From f8b02f7b454eb913dbcc8eb572bd2cb409dd0af8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 12 Mar 2025 19:29:02 -0700 Subject: [PATCH 07/10] Add comment for IsSync --- services/pull/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 615c032ab0f3e..05f7d57301be5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -376,7 +376,7 @@ type TestPullRequestOptions struct { RepoID int64 Doer *user_model.User Branch string - IsSync bool + IsSync bool // whether it's a pull request synchronization IsForcePush bool OldCommitID string NewCommitID string From 2aee5649a6324976350c64ed8678db787de3b94d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 12 Mar 2025 20:47:15 -0700 Subject: [PATCH 08/10] Fix test --- tests/integration/pull_review_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 96cab3fee65a2..7b69223a44e25 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -116,8 +116,7 @@ func TestPullView_CodeOwner(t *testing.T) { reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr) assert.NoError(t, err) assert.Len(t, reviewNotifiers, 2) - assert.EqualValues(t, 5, reviewNotifiers[0].Reviewer.ID) - assert.EqualValues(t, 8, reviewNotifiers[1].Reviewer.ID) + assert.EqualValues(t, []int64{5, 8}, []int64{reviewNotifiers[0].Reviewer.ID, reviewNotifiers[1].Reviewer.ID}) reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(db.DefaultContext, pr, resp1.Commit.SHA, resp2.Commit.SHA) assert.NoError(t, err) From 4961418dda6166c9d14cd9676121bf3da6eb5bfe Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 13 Mar 2025 15:59:08 -0700 Subject: [PATCH 09/10] improve the comment for IsSync --- services/pull/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 05f7d57301be5..21f31e16cf07a 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -376,7 +376,7 @@ type TestPullRequestOptions struct { RepoID int64 Doer *user_model.User Branch string - IsSync bool // whether it's a pull request synchronization + IsSync bool // True means it's a pull request synchronization, false means it's triggered for pull request merging or updating IsForcePush bool OldCommitID string NewCommitID string From a9502815b79175955699802fdabb7ce9eac8be9a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 13 Mar 2025 18:30:53 -0700 Subject: [PATCH 10/10] Fix test --- tests/integration/pull_review_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 7b69223a44e25..cd354e1b6cf26 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "net/url" "path" + "sort" "strings" "testing" @@ -116,7 +117,9 @@ func TestPullView_CodeOwner(t *testing.T) { reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr) assert.NoError(t, err) assert.Len(t, reviewNotifiers, 2) - assert.EqualValues(t, []int64{5, 8}, []int64{reviewNotifiers[0].Reviewer.ID, reviewNotifiers[1].Reviewer.ID}) + reviewerIDs := []int64{reviewNotifiers[0].Reviewer.ID, reviewNotifiers[1].Reviewer.ID} + sort.Slice(reviewerIDs, func(i, j int) bool { return reviewerIDs[i] < reviewerIDs[j] }) + assert.EqualValues(t, []int64{5, 8}, reviewerIDs) reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(db.DefaultContext, pr, resp1.Commit.SHA, resp2.Commit.SHA) assert.NoError(t, err)