From e057cdf0d3dcce84c13aec9b5d8e0a57dff769eb003a9dea05148fe164d98873 Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Thu, 12 Dec 2024 19:16:32 +0100 Subject: [PATCH] workflow-pr: review processing --- workflow-pr/review.go | 108 +++++++------- workflow-pr/review_test.go | 282 ++++++++++++++++++++++++++++++++----- 2 files changed, 300 insertions(+), 90 deletions(-) diff --git a/workflow-pr/review.go b/workflow-pr/review.go index 0f2b034..756070c 100644 --- a/workflow-pr/review.go +++ b/workflow-pr/review.go @@ -2,7 +2,6 @@ package main import ( "bufio" - "fmt" "slices" "strings" @@ -10,77 +9,82 @@ import ( "src.opensuse.org/autogits/common/gitea-generated/models" ) -type PRReviewInfo struct { +type Review interface { + IsApproved() (bool, error) +} + +type PRInfo struct { pr *models.PullRequest reviews []*models.PullReview } -func fetchPRandReviews(gitea GiteaPRInterface, org, repo string, prNum int64) (PRReviewInfo, error) { - pr, reviews, err := gitea.GetPullRequestAndReviews(org, repo, prNum) - if err != nil { - return PRReviewInfo{}, err - } +type ReviewSet struct { + maintainers MaintainershipData + prs []PRInfo +} - return PRReviewInfo{ - pr: pr, +func fetchPRInfo(gitea GiteaPRInterface, pr common.BasicPR) PRInfo { + data, reviews, _ := gitea.GetPullRequestAndReviews(pr.Org, pr.Repo, pr.Num) + return PRInfo{ + pr: data, reviews: reviews, - }, nil + } } -func isMaintainerApprovedPR(pr PRReviewInfo, maintainers MaintainershipData) bool { - m := slices.Concat(maintainers.ListPackageMaintainers(pr.pr.Base.Name), maintainers.ListProjectMaintainers()) - for _, review := range pr.reviews { - if review.Stale { - continue - } - - if slices.Contains(m, review.User.UserName) { - if review.State == common.ReviewStateApproved { - return true - } - return false - } +func (rs *ReviewSet) appendPR(gitea GiteaPRInterface, pr common.BasicPR) { + if slices.ContainsFunc(rs.prs, func(elem PRInfo) bool { + return pr.Org == elem.pr.Base.Repo.Owner.UserName && + pr.Repo == elem.pr.Base.Repo.Name && + pr.Num == elem.pr.Index + }) { + return } - return true + prinfo := fetchPRInfo(gitea, pr) + rs.prs = append(rs.prs, prinfo) + _, childPRs := common.ExtractDescriptionAndPRs(bufio.NewScanner(strings.NewReader(prinfo.pr.Body))) + for _, childPR := range childPRs { + rs.appendPR(gitea, childPR) + } } -func IsPrjGitPRApproved(gitea common.GiteaMaintainershipInterface, giteapr GiteaPRInterface, config common.AutogitConfig, prjGitPRNumber int64) (bool, error) { - prjPR, _ := fetchPRandReviews(giteapr, config.Organization, config.GitProjectName, prjGitPRNumber) +func NewReviewInstance(gitea GiteaPRInterface, maintainers MaintainershipData, org, repo string, prNum int) (*ReviewSet, error) { + ret := &ReviewSet{ + maintainers: maintainers, + prs: []PRInfo{}, + } - maintainers, _ := FetchProjectMaintainershipData(gitea, config.Organization, config.GitProjectName, config.Branch) + ret.appendPR(gitea, common.BasicPR{Org: org, Repo: repo, Num: int64(prNum)}) + return ret, nil +} - _, prjAssociatedPRs := common.ExtractDescriptionAndPRs(bufio.NewScanner(strings.NewReader(prjPR.pr.Body))) +func isReviewMaintainerApproved(pkgMaintainersList []string, review *models.PullReview) bool { + if review.State != common.ReviewStateApproved || review.Stale { + return false; + } - for _, PR := range prjAssociatedPRs { - prInfo, _ := fetchPRandReviews(giteapr, PR.Org, PR.Repo, PR.Num) + if slices.Contains(pkgMaintainersList, review.User.UserName) { + return true + } - _, associatedPRs := common.ExtractDescriptionAndPRs(bufio.NewScanner(strings.NewReader(prInfo.pr.Body))) + return false; +} - if len(associatedPRs) != 1 { - return false, fmt.Errorf("Associated PR doesn't link only to the prjgit PR: %s/%s#%d", - associatedPRs[0].Org, associatedPRs[0].Repo, associatedPRs[0].Num) +func (prinfo *PRInfo) isMaintainerApproved(pkgMaintainersList []string) bool { + for _, review := range prinfo.reviews { + if isReviewMaintainerApproved(pkgMaintainersList, review) { + return true } + } + return false +} - if associatedPRs[0].Org != config.Organization || associatedPRs[0].Repo != config.GitProjectName || associatedPRs[0].Num != prjGitPRNumber { - return false, fmt.Errorf("Associated PR (%s/%s#%d) not linking back to prj PR (%s/%s#%d)", - associatedPRs[0].Org, associatedPRs[0].Repo, associatedPRs[0].Num, - config.Organization, config.GitProjectName, prjGitPRNumber) - } - - if !isMaintainerApprovedPR(prInfo, maintainers) { +func (rs *ReviewSet) IsMaintainerApproved() (bool, error) { + for _, prinfo := range rs.prs { + pkgMaintainerList := rs.maintainers.ListPackageMaintainers(prinfo.pr.Base.Repo.Name) + if !prinfo.isMaintainerApproved(pkgMaintainerList) { return false, nil } } - - requiredReviews := slices.Clone(config.Reviewers) - for _, r := range prjPR.reviews { - if !r.Stale && r.State == common.ReviewStateApproved && slices.Contains(requiredReviews, r.User.UserName) { - idx := slices.Index(requiredReviews, r.User.UserName) - requiredReviews = slices.Delete(requiredReviews, idx, idx+1) - } - } - - return len(requiredReviews) == 0, nil + return true, nil } - diff --git a/workflow-pr/review_test.go b/workflow-pr/review_test.go index ee8a24d..0ac89c1 100644 --- a/workflow-pr/review_test.go +++ b/workflow-pr/review_test.go @@ -1,55 +1,257 @@ package main import ( + "bufio" + "strings" "testing" "go.uber.org/mock/gomock" "src.opensuse.org/autogits/common" "src.opensuse.org/autogits/common/gitea-generated/models" - mock_common "src.opensuse.org/autogits/common/mock" mock_main "src.opensuse.org/workflow-pr/mock" ) +type pr_review map[string]struct { + pr *models.PullRequest + reviews []*models.PullReview + maintainers []string +} + func TestReviewApproval(t *testing.T) { - config := common.AutogitConfig{ - Branch: "bar", - Organization: "foo", - GitProjectName: common.DefaultGitPrj, - } - tests := []struct { - name string - pr *models.PullRequest - reviews []*models.PullReview - - maintainerFile []byte + name string + prs pr_review approved bool }{ { - name: "Maintainer not approved", - pr: &models.PullRequest{Body: "PR: foo/foo#10", Index: 10, RequestedReviewers: []*models.User{}}, - reviews: []*models.PullReview{}, - - maintainerFile: []byte(`{"foo": ["bingo"]}`), + name: "Maintainer not approved", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "nothing PR: foo/foo#10", Index: 10, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, + }, + reviews: []*models.PullReview{}, + maintainers: []string{"bingo"}, + }, + }, approved: false, }, { name: "Maintainer approved", - pr: &models.PullRequest{Body: "", Index: 10, RequestedReviewers: []*models.User{}}, - reviews: []*models.PullReview{ - &models.PullReview{ - Body: "wow!", - Stale: false, - State: common.ReviewStateApproved, - User: &models.User{ - UserName: "king", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "", Index: 42, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateApproved, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, }, }, - maintainerFile: []byte(`{"": ["king"], "foo": ["bingo"]}`), + approved: true, + }, + { + name: "Maintainer approval is missing", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "", Index: 42, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateApproved, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"kong", "bingo"}, + }, + }, + + approved: false, + }, + { + name: "Maintainer dis-approved", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "", Index: 42, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateRequestChanges, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, + }, + }, + + approved: false, + }, + { + name: "Maintainer review is stale", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "", Index: 42, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + Stale: true, + State: common.ReviewStateApproved, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, + }, + }, + + approved: false, + }, + { + name: "Part of ReviewSet is not approved", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "PR: foo/bar#10", Index: 42, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateApproved, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, + }, + "foo/bar#10": { + pr: &models.PullRequest{ + Body: "PR: org/repo#42", Index: 10, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "bar"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateRequestChanges, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, + }, + }, + + approved: false, + }, + { + name: "Review set approved", + prs: pr_review{ + "org/repo#42": { + pr: &models.PullRequest{ + Body: "PR: foo/bar#10", Index: 42, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "repo"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateApproved, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, + }, + "foo/bar#10": { + pr: &models.PullRequest{ + Body: "PR: org/repo#42", Index: 10, + RequestedReviewers: []*models.User{}, + Base: &models.PRBranchInfo{ + Repo: &models.Repository{ + Owner: &models.User{UserName: "org"}, + Name: "bar"}, + }, + }, + reviews: []*models.PullReview{ + &models.PullReview{ + Body: "wow!", + State: common.ReviewStateApproved, + User: &models.User{ + UserName: "king", + }, + }, + }, + maintainers: []string{"king", "bingo"}, + }, + }, approved: true, }, @@ -58,21 +260,25 @@ func TestReviewApproval(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { ctl := gomock.NewController(t) - mi := mock_common.NewMockGiteaMaintainershipInterface(ctl) - pri := mock_main.NewMockGiteaPRInterface(ctl) + pr := mock_main.NewMockGiteaPRInterface(ctl) + maintainership := mock_main.NewMockMaintainershipData(ctl) - pri.EXPECT().GetPullRequestAndReviews("foo", common.DefaultGitPrj, int64(10)). - Return(test.pr, test.reviews, nil) - mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(test.maintainerFile, nil) - - approved, err := IsPrjGitPRApproved(mi, pri, config, 10) - if approved != test.approved { - t.Error("Unexpected approve state:", approved, "vs. expected", test.approved, ", or err:", err) + for params, prs := range test.prs { + _, data := common.ExtractDescriptionAndPRs(bufio.NewScanner(strings.NewReader("PR: " + params))) + if len(data) != 1 { + t.Fatal("bad test setup, fix") + } + pr.EXPECT().GetPullRequestAndReviews(data[0].Org, data[0].Repo, data[0].Num). + Return(prs.pr, prs.reviews, nil) + maintainership.EXPECT().ListPackageMaintainers(data[0].Repo).Return(prs.maintainers) } - if err != nil { - t.Error("Unexpected error", err) + + info, _ := NewReviewInstance(pr, maintainership, "org", "repo", 42) + + approved, _ := info.IsMaintainerApproved() + if test.approved != approved { + t.Error("Unexpected approval state:", approved) } }) } } -