From 5f00b10f35e9fadc84bbc4d1bd44632635536fa7dfe3bec6997ac26469621940 Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Mon, 9 Dec 2024 18:20:56 +0100 Subject: [PATCH] wip --- bots-common/associated_pr_scanner.go | 2 +- bots-common/associated_pr_scanner_test.go | 2 +- workflow-pr/maintainership.go | 64 ++++++++++----- workflow-pr/maintainership_test.go | 98 +++++++++++++++-------- 4 files changed, 109 insertions(+), 57 deletions(-) diff --git a/bots-common/associated_pr_scanner.go b/bots-common/associated_pr_scanner.go index 856d3ef..a1f9c16 100644 --- a/bots-common/associated_pr_scanner.go +++ b/bots-common/associated_pr_scanner.go @@ -55,7 +55,7 @@ func parsePrLine(line string) (BasicPR, error) { return ret, nil } -func ExtractPRsFromDescription(data *bufio.Scanner) (string, []BasicPR) { +func ExtractDescriptionAndPRs(data *bufio.Scanner) (string, []BasicPR) { prs := make([]BasicPR, 0, 1) var desc strings.Builder diff --git a/bots-common/associated_pr_scanner_test.go b/bots-common/associated_pr_scanner_test.go index 94731f1..4b56871 100644 --- a/bots-common/associated_pr_scanner_test.go +++ b/bots-common/associated_pr_scanner_test.go @@ -73,7 +73,7 @@ func TestAssociatedPRScanner(t *testing.T) { for _, test := range testTable { t.Run(test.name, func(t *testing.T) { - desc, prs := ExtractPRsFromDescription(newStringScanner(test.input)) + desc, prs := ExtractDescriptionAndPRs(newStringScanner(test.input)) if len(prs) != len(test.prs) { t.Error("Unexpected length:", len(prs), "expected:", len(test.prs)) return diff --git a/workflow-pr/maintainership.go b/workflow-pr/maintainership.go index f95a7e7..4e73a53 100644 --- a/workflow-pr/maintainership.go +++ b/workflow-pr/maintainership.go @@ -4,6 +4,7 @@ import ( "bufio" "encoding/json" "fmt" + "slices" "strings" "src.opensuse.org/autogits/common" @@ -65,45 +66,64 @@ type PRReviewInfo struct { reviews []*models.PullReview } -func fetchAllAssociatedPRs(gitea GiteaPRInterface, org, repo string, prNum int64) ([]PRReviewInfo, error) { +func fetchPRandReviews(gitea GiteaPRInterface, org, repo string, prNum int64) (PRReviewInfo, error) { pr, reviews, err := gitea.GetPullRequestAndReviews(org, repo, prNum) if err != nil { - return nil, err + return PRReviewInfo{}, err } - ret := make([]PRReviewInfo, 1, 2) - ret[0].pr = pr - ret[0].reviews = reviews - - return ret, nil + return PRReviewInfo{ + pr: pr, + reviews: reviews, + }, nil } -func CheckIfMaintainersApproved(gitea common.GiteaMaintainershipInterface, giteapr GiteaPRInterface, config common.AutogitConfig, prjGitPRNumber int64) (bool, error) { - prs, _ := fetchAllAssociatedPRs(giteapr, config.Organization, config.GitProjectName, prjGitPRNumber) - data, _ := gitea.FetchMaintainershipFile(config.Organization, config.GitProjectName, config.Branch) - - for _, pr := range prs { - _, associatedPRs := common.ExtractPRsFromDescription(bufio.NewScanner(strings.NewReader(pr.pr.Body))) - - if len(associatedPRs) == 0 { - // no associated packages with this PR - break +func isMaintainerApprovedPR(pr PRReviewInfo, maintainers MaintainershipMap) bool { + m := append(MaintainerListForPackage(maintainers, pr.pr.Base.Name), MaintainerListForProject(maintainers)...) + 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 + } + } + + return true +} + +func IsPrjGitPRApproved(gitea common.GiteaMaintainershipInterface, giteapr GiteaPRInterface, config common.AutogitConfig, prjGitPRNumber int64) (bool, error) { + prjPR, _ := fetchPRandReviews(giteapr, config.Organization, config.GitProjectName, prjGitPRNumber) + + data, _ := gitea.FetchMaintainershipFile(config.Organization, config.GitProjectName, config.Branch) + maintainers, _ := parseMaintainershipData(data) + + _, prjAssociatedPRs := common.ExtractDescriptionAndPRs(bufio.NewScanner(strings.NewReader(prjPR.pr.Body))) + + for _, PR := range prjAssociatedPRs { + prInfo, _ := fetchPRandReviews(giteapr, PR.Org, PR.Repo, PR.Num) + + _, associatedPRs := common.ExtractDescriptionAndPRs(bufio.NewScanner(strings.NewReader(prInfo.pr.Body))) + 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) - } 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)", + 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) { + return false, nil + } } - parseMaintainershipData(data) - - return false, nil + return true, nil } diff --git a/workflow-pr/maintainership_test.go b/workflow-pr/maintainership_test.go index 7ff521b..05e159a 100644 --- a/workflow-pr/maintainership_test.go +++ b/workflow-pr/maintainership_test.go @@ -12,7 +12,6 @@ import ( ) func TestMaintainership(t *testing.T) { - allocateMaintainershipInterface := func(t *testing.T) (*mock_common.MockGiteaMaintainershipInterface, *mock_main.MockGiteaPRInterface) { ctl := gomock.NewController(t) mi := mock_common.NewMockGiteaMaintainershipInterface(ctl) @@ -27,41 +26,58 @@ func TestMaintainership(t *testing.T) { GitProjectName: common.DefaultGitPrj, } - t.Run("No maintainer in empty package", func(t *testing.T) { - mi, _ := allocateMaintainershipInterface(t) + packageTests := []struct { + name string + }{ + { + name: "No maintainer in empty package", + }, + } - mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, nil) + for _, test := range packageTests { + t.Run(test.name, func(t *testing.T) { + mi, _ := allocateMaintainershipInterface(t) + mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, nil) + maintainers, err := ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) + if err != nil { + t.Error("Invalid number of maintainers for package", err) + } - maintainers, err := ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) - if err != nil { - t.Error("Invalid number of maintainers for package", err) - } + m := MaintainerListForPackage(maintainers, "bar") + if len(m) != 0 { + t.Error("Invalid number of maintainers for package", err) + } + }) + } - m := MaintainerListForPackage(maintainers, "bar") - if len(m) != 0 { - t.Error("Invalid number of maintainers for package", err) - } - }) + projectTests := []struct { + name string + maintainersFile []byte + }{ + { + name: "No maintainer for empty project", + }, + { + name: "No maintainer for empty project maintainer file", + maintainersFile: []byte("{}"), + }, + } - t.Run("No maintainer for empty projects", func(t *testing.T) { - mi, _ := allocateMaintainershipInterface(t) + for _, test := range projectTests { + t.Run(test.name, func(t *testing.T) { + mi, _ := allocateMaintainershipInterface(t) + mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(test.maintainersFile, nil) + maintainers, err := ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) + if err != nil { + t.Error("Invalid number of maintainers for package", err) + } - mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, nil) - - maintainers, err := ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) - m := MaintainerListForProject(maintainers) - if len(m) != 0 || err != nil { - t.Error("Invalid number of maintainers for project", err) - } - - mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte("{}"), nil) - - maintainers, err = ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) - m = MaintainerListForProject(maintainers) - if len(m) != 0 || err != nil { - t.Error("Invalid number of maintainers for project", err) - } - }) + m := MaintainerListForProject(maintainers) + if len(m) != 0 { + t.Error("Invalid number of maintainers for project", err) + } + }) + } t.Run("Error in MaintainerListForPackage when remote has an error", func(t *testing.T) { mi, _ := allocateMaintainershipInterface(t) @@ -237,6 +253,22 @@ func TestMaintainership(t *testing.T) { t.Error("Invalid number of maintainers for project", err) } }) +} + +func TestReviewApproval(t *testing.T) { + allocateMaintainershipInterface := func(t *testing.T) (*mock_common.MockGiteaMaintainershipInterface, *mock_main.MockGiteaPRInterface) { + ctl := gomock.NewController(t) + mi := mock_common.NewMockGiteaMaintainershipInterface(ctl) + pri := mock_main.NewMockGiteaPRInterface(ctl) + + return mi, pri + } + + config := common.AutogitConfig{ + Branch: "bar", + Organization: "foo", + GitProjectName: common.DefaultGitPrj, + } t.Run("Maintainers not appoved", func(t *testing.T) { mi, pri := allocateMaintainershipInterface(t) @@ -252,7 +284,7 @@ func TestMaintainership(t *testing.T) { ) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(`{"foo": ["bingo"]}`), nil) - approved, err := CheckIfMaintainersApproved(mi, pri, config, 10) + approved, err := IsPrjGitPRApproved(mi, pri, config, 10) if approved || err != nil { t.Error("Unexpected approved or err:", err) } @@ -274,7 +306,7 @@ func TestMaintainership(t *testing.T) { "": ["test"] `), nil) - approved, err := CheckIfMaintainersApproved(mi, pri, config, 10) + approved, err := IsPrjGitPRApproved(mi, pri, config, 10) if !approved || err != nil { t.Error("Unexpected disapproval or err:", err) }