From 1e7248047ce66c235faf53cd79b6513fb3aa1c50 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 13 Mar 2025 19:36:14 -0700 Subject: [PATCH] Pull request updates will also trigger code owners review requests (#33744) Fix #33490 It will only read the changed file on the pushed commits but not all the files of this PR. --------- Co-authored-by: wxiaoguang --- routers/web/repo/view_file.go | 3 +- services/issue/issue.go | 6 ++- services/issue/pull.go | 42 ++++++++++++++----- services/pull/merge.go | 10 ++++- services/pull/pull.go | 59 +++++++++++++++++++-------- services/pull/update.go | 20 ++++++++- services/repository/push.go | 12 +++++- tests/integration/pull_review_test.go | 42 +++++++++++++++++-- 8 files changed, 157 insertions(+), 37 deletions(-) diff --git a/routers/web/repo/view_file.go b/routers/web/repo/view_file.go index 5784297918..4ce7a8e3a4 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/issue.go b/services/issue/issue.go index 586b6031c8..455a1ec297 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 896802108d..bd19c25436 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,20 +41,31 @@ 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"} +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) { + 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) { + 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 } + pr.Issue.Repo = pr.BaseRepo if pr.BaseRepo.IsFork { return nil, nil @@ -71,7 +83,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, } var data string - 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 { @@ -79,18 +91,28 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, } } } + 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 665909e99a..1e1ca55bc1 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, "", "") + 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 1f06d6b182..21f31e16cf 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 } @@ -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 // 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 +} + // 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) { - 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,24 +410,24 @@ 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 { if err = prs.LoadAttributes(ctx); err != nil { log.Error("PullRequestList.LoadAttributes: %v", err) } - if invalidationErr := checkForInvalidation(ctx, prs, repoID, doer, branch); invalidationErr != nil { + if invalidationErr := checkForInvalidation(ctx, prs, 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) } @@ -433,12 +443,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) @@ -452,15 +462,30 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } } - notify_service.PullRequestSynchronized(ctx, doer, pr) + if !pr.IsWorkInProgress(ctx) { + var reviewNotifiers []*issue_service.ReviewRequestNotifier + if opts.IsForcePush { + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) + } else { + 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, opts.Doer, reviewNotifiers) + } + } + + 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 abf7ad4509..3e00dd4e65 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, "", "") + 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, "", "") + 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 0ea51f9c07..00d4fb11af 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,17 @@ 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(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) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 68de421413..cd354e1b6c 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" @@ -67,7 +68,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 +76,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 +96,37 @@ 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) + 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) + 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 +171,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) {