From 7ccbd1deb28dbe3e4742f07e0092158f09641540062d75dbbc27d2656473bbeb Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Wed, 11 Dec 2024 14:41:51 +0100 Subject: [PATCH] wip --- workflow-pr/maintainership.go | 108 +++++----------------- workflow-pr/maintainership_test.go | 140 +++++------------------------ workflow-pr/review.go | 86 ++++++++++++++++++ workflow-pr/review_test.go | 78 ++++++++++++++++ 4 files changed, 207 insertions(+), 205 deletions(-) create mode 100644 workflow-pr/review.go create mode 100644 workflow-pr/review_test.go diff --git a/workflow-pr/maintainership.go b/workflow-pr/maintainership.go index 05f3519..d5758bc 100644 --- a/workflow-pr/maintainership.go +++ b/workflow-pr/maintainership.go @@ -1,32 +1,32 @@ package main import ( - "bufio" "encoding/json" - "fmt" - "slices" - "strings" "src.opensuse.org/autogits/common" - "src.opensuse.org/autogits/common/gitea-generated/models" ) //go:generate mockgen -source=maintainership.go -destination=mock/maintainership.go -typed +type MaintainershipData interface { + ListProjectMaintainers() []string + ListPackageMaintainers(pkg string) []string +} + const ProjectKey = "" type MaintainershipMap map[string][]string -func parseMaintainershipData(data []byte) (MaintainershipMap, error) { +func parseMaintainershipData(data []byte) (*MaintainershipMap, error) { maintainers := make(MaintainershipMap) if err := json.Unmarshal(data, &maintainers); err != nil { return nil, err } - return maintainers, nil + return &maintainers, nil } -func ProjectMaintainershipData(gitea common.GiteaMaintainershipInterface, org, prjGit, branch string) (MaintainershipMap, error) { +func FetchProjectMaintainershipData(gitea common.GiteaMaintainershipInterface, org, prjGit, branch string) (*MaintainershipMap, error) { data, err := gitea.FetchMaintainershipFile(org, prjGit, branch) if err != nil || data == nil { return nil, err @@ -35,8 +35,12 @@ func ProjectMaintainershipData(gitea common.GiteaMaintainershipInterface, org, p return parseMaintainershipData(data) } -func MaintainerListForProject(maintainers MaintainershipMap) []string { - m, found := maintainers[ProjectKey] +func (data *MaintainershipMap) ListProjectMaintainers() []string { + if data == nil { + return nil + } + + m, found := (*data)[ProjectKey] if !found { return nil } @@ -44,9 +48,13 @@ func MaintainerListForProject(maintainers MaintainershipMap) []string { return m } -func MaintainerListForPackage(maintainers MaintainershipMap, pkg string) []string { - pkgMaintainers := maintainers[pkg] - prjMaintainers := maintainers[ProjectKey] +func (data *MaintainershipMap) ListPackageMaintainers(pkg string) []string { + if data == nil { + return nil + } + + pkgMaintainers := (*data)[pkg] + prjMaintainers := data.ListProjectMaintainers() prjMaintainer: for _, prjm := range prjMaintainers { @@ -61,77 +69,3 @@ prjMaintainer: return pkgMaintainers } -type PRReviewInfo 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 - } - - return PRReviewInfo{ - pr: pr, - reviews: reviews, - }, nil -} - -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)", - associatedPRs[0].Org, associatedPRs[0].Repo, associatedPRs[0].Num, - config.Organization, config.GitProjectName, prjGitPRNumber) - } - - if !isMaintainerApprovedPR(prInfo, maintainers) { - 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 -} diff --git a/workflow-pr/maintainership_test.go b/workflow-pr/maintainership_test.go index 6ffeffc..d31f2d9 100644 --- a/workflow-pr/maintainership_test.go +++ b/workflow-pr/maintainership_test.go @@ -7,9 +7,7 @@ import ( "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" ) func TestMaintainership(t *testing.T) { @@ -25,72 +23,40 @@ func TestMaintainership(t *testing.T) { maintainersFileErr error maintainers []string otherError bool + packageName string }{ + /* PACKAGE MAINTAINERS */ + // package tests have packageName, projects do not { name: "No maintainer in empty package", + packageName: "foo", }, { name: "Error in MaintainerListForPackage when remote has an error", maintainersFileErr: errors.New("some error here"), + packageName: "foo", }, { name: "Multiple package maintainers", maintainersFile: []byte(`{"pkg": ["user1", "user2"], "": ["user1", "user3"]}`), maintainers: []string{"user1", "user2", "user3"}, + packageName: "pkg", }, { name: "No package maintainers and only project maintainer", maintainersFile: []byte(`{"pkg2": ["user1", "user2"], "": ["user1", "user3"]}`), maintainers: []string{"user1", "user3"}, + packageName: "pkg", }, { name: "Invalid list of package maintainers", maintainersFile: []byte(`{"pkg": 3,"": ["user", 4]}`), otherError: true, + packageName: "pkg", }, - } - for _, test := range packageTests { - t.Run(test.name, func(t *testing.T) { - ctl := gomock.NewController(t) - mi := mock_common.NewMockGiteaMaintainershipInterface(ctl) - mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar"). - Return(test.maintainersFile, test.maintainersFileErr) - - maintainers, err := ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) - if err != nil && !test.otherError { - if test.maintainersFileErr == nil { - t.Fatal("Unexpected error recieved", err) - } else if err != test.maintainersFileErr { - t.Error("Unexpected error recieved", err) - } - } else if test.maintainersFileErr != nil { - t.Fatal("Expected an error...") - } else if test.otherError && err == nil { - t.Fatal("Expected an error...") - } - - m := MaintainerListForPackage(maintainers, "pkg") - if len(m) != len(test.maintainers) { - t.Error("Invalid number of maintainers for package", err) - } - for i := range m { - if !slices.Contains(test.maintainers, m[i]) { - t.Fatal("Can't find expected users. Found:", m) - } - } - }) - } - - projectTests := []struct { - name string - maintainersFile []byte - maintainersFileErr error - numMaintainers int - maintainers []string - otherError bool - }{ + /* PROJECT MAINTAINERS */ { name: "No maintainer for empty project", }, @@ -105,13 +71,11 @@ func TestMaintainership(t *testing.T) { { name: "Multiple project maintainers", maintainersFile: []byte(`{"": ["user1", "user2"]}`), - numMaintainers: 2, maintainers: []string{"user1", "user2"}, }, { name: "Single project maintainer", maintainersFile: []byte(`{"": ["user"]}`), - numMaintainers: 1, maintainers: []string{"user"}, }, { @@ -126,7 +90,7 @@ func TestMaintainership(t *testing.T) { }, } - for _, test := range projectTests { + for _, test := range packageTests { t.Run(test.name, func(t *testing.T) { ctl := gomock.NewController(t) mi := mock_common.NewMockGiteaMaintainershipInterface(ctl) @@ -134,12 +98,12 @@ func TestMaintainership(t *testing.T) { mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar"). Return(test.maintainersFile, test.maintainersFileErr) - maintainers, err := ProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) + maintainers, err := FetchProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) if err != nil && !test.otherError { if test.maintainersFileErr == nil { t.Fatal("Unexpected error recieved", err) } else if err != test.maintainersFileErr { - t.Error("Unexpected error recieved", err) + t.Error("Wrong error recieved", err) } } else if test.maintainersFileErr != nil { t.Fatal("Expected an error...") @@ -147,82 +111,22 @@ func TestMaintainership(t *testing.T) { t.Fatal("Expected an error...") } - m := MaintainerListForProject(maintainers) - if len(m) != test.numMaintainers { - t.Error("Invalid number of maintainers", err) + var m []string + if len(test.packageName) > 0 { + m = maintainers.ListPackageMaintainers(test.packageName) + } else { + m = maintainers.ListProjectMaintainers() } + if len(m) != len(test.maintainers) { + t.Error("Invalid number of maintainers for package", err) + } for i := range m { - if i >= len(test.maintainers) || test.maintainers[i] != m[i] { - t.Error("Can't find expected users. Found:", m) + if !slices.Contains(test.maintainers, m[i]) { + t.Fatal("Can't find expected users. Found:", m) } } }) } } -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 - - 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"]}`), - - 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", - }, - }, - }, - - maintainerFile: []byte(`{"": ["king"], "foo": ["bingo"]}`), - - approved: true, - }, - } - - 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) - - 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) - } - if err != nil { - t.Error("Unexpected error", err) - } - }) - } -} diff --git a/workflow-pr/review.go b/workflow-pr/review.go new file mode 100644 index 0000000..0f2b034 --- /dev/null +++ b/workflow-pr/review.go @@ -0,0 +1,86 @@ +package main + +import ( + "bufio" + "fmt" + "slices" + "strings" + + "src.opensuse.org/autogits/common" + "src.opensuse.org/autogits/common/gitea-generated/models" +) + +type PRReviewInfo 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 + } + + return PRReviewInfo{ + pr: pr, + 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 + } + } + + return true +} + +func IsPrjGitPRApproved(gitea common.GiteaMaintainershipInterface, giteapr GiteaPRInterface, config common.AutogitConfig, prjGitPRNumber int64) (bool, error) { + prjPR, _ := fetchPRandReviews(giteapr, config.Organization, config.GitProjectName, prjGitPRNumber) + + maintainers, _ := FetchProjectMaintainershipData(gitea, config.Organization, config.GitProjectName, config.Branch) + + _, 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)", + associatedPRs[0].Org, associatedPRs[0].Repo, associatedPRs[0].Num, + config.Organization, config.GitProjectName, prjGitPRNumber) + } + + if !isMaintainerApprovedPR(prInfo, maintainers) { + 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 +} + diff --git a/workflow-pr/review_test.go b/workflow-pr/review_test.go new file mode 100644 index 0000000..ee8a24d --- /dev/null +++ b/workflow-pr/review_test.go @@ -0,0 +1,78 @@ +package main + +import ( + "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" +) + +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 + + 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"]}`), + + 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", + }, + }, + }, + + maintainerFile: []byte(`{"": ["king"], "foo": ["bingo"]}`), + + approved: true, + }, + } + + 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) + + 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) + } + if err != nil { + t.Error("Unexpected error", err) + } + }) + } +} +