diff --git a/bots-common/associated_pr_scanner.go b/bots-common/associated_pr_scanner.go index 6d7c097..856d3ef 100644 --- a/bots-common/associated_pr_scanner.go +++ b/bots-common/associated_pr_scanner.go @@ -14,8 +14,8 @@ import ( const PrPattern = "PR: %s/%s#%d" type BasicPR struct { - org, repo string - num uint64 + Org, Repo string + Num int64 } var validOrgAndRepoRx *regexp.Regexp = regexp.MustCompile("^[A-Za-z0-9_-]+$") @@ -31,20 +31,20 @@ func parsePrLine(line string) (BasicPR, error) { trimmedLine = trimmedLine[4:] org := strings.SplitN(trimmedLine, "/", 2) - ret.org = org[0] + ret.Org = org[0] if len(org) != 2 { return ret, errors.New("missing / separator") } repo := strings.SplitN(org[1], "#", 2) - ret.repo = repo[0] + ret.Repo = repo[0] if len(repo) != 2 { return ret, errors.New("Missing # separator") } // Gitea requires that each org and repo be [A-Za-z0-9_-]+ var err error - if ret.num, err = strconv.ParseUint(repo[1], 10, 64); err != nil { + if ret.Num, err = strconv.ParseInt(repo[1], 10, 64); err != nil { return ret, errors.New("Invalid number") } @@ -76,7 +76,7 @@ func ExtractPRsFromDescription(data *bufio.Scanner) (string, []BasicPR) { func prToLine(writer io.Writer, pr BasicPR) { writer.Write([]byte("\n")) - fmt.Fprintf(writer, PrPattern, pr.org, pr.repo, pr.num) + fmt.Fprintf(writer, PrPattern, pr.Org, pr.Repo, pr.Num) } // returns: @@ -84,17 +84,17 @@ func prToLine(writer io.Writer, pr BasicPR) { // >0 for a>b // =0 when equal func compareBasicPRs(a BasicPR, b BasicPR) int { - if c := strings.Compare(a.org, b.org); c != 0 { + if c := strings.Compare(a.Org, b.Org); c != 0 { return c } - if c := strings.Compare(a.repo, b.repo); c != 0 { + if c := strings.Compare(a.Repo, b.Repo); c != 0 { return c } - if a.num > b.num { + if a.Num > b.Num { return 1 } - if a.num < b.num { + if a.Num < b.Num { return -1 } diff --git a/bots-common/associated_pr_scanner_test.go b/bots-common/associated_pr_scanner_test.go index ce1664d..94731f1 100644 --- a/bots-common/associated_pr_scanner_test.go +++ b/bots-common/associated_pr_scanner_test.go @@ -27,15 +27,15 @@ func TestAssociatedPRScanner(t *testing.T) { { "Single PRs", "Some header of the issue\n\nFollowed by some description\n\nPR: test/foo#4\n", - []BasicPR{{org: "test", repo: "foo", num: 4}}, + []BasicPR{{Org: "test", Repo: "foo", Num: 4}}, "Some header of the issue\n\nFollowed by some description", }, { "Multiple PRs", "Some header of the issue\n\nFollowed by some description\nPR: test/foo#4\n\nPR: test/goo#5\n", []BasicPR{ - {org: "test", repo: "foo", num: 4}, - {org: "test", repo: "goo", num: 5}, + {Org: "test", Repo: "foo", Num: 4}, + {Org: "test", Repo: "goo", Num: 5}, }, "Some header of the issue\n\nFollowed by some description", }, @@ -43,8 +43,8 @@ func TestAssociatedPRScanner(t *testing.T) { "Multiple PRs with whitespace", "Some header of the issue\n\n\tPR: test/goo#5\n\n Followed by some description\n \t PR: test/foo#4\n", []BasicPR{ - {org: "test", repo: "foo", num: 4}, - {org: "test", repo: "goo", num: 5}, + {Org: "test", Repo: "foo", Num: 4}, + {Org: "test", Repo: "goo", Num: 5}, }, "Some header of the issue\n\n\n Followed by some description", }, @@ -55,14 +55,14 @@ func TestAssociatedPRScanner(t *testing.T) { "\t PR: test/goo#5 \n\t\n Followed by some description\n\t PR: test/foo#4 \n\t\n\n", []BasicPR{ { - org: "test", - repo: "foo", - num: 4, + Org: "test", + Repo: "foo", + Num: 4, }, { - org: "test", - repo: "goo", - num: 5, + Org: "test", + Repo: "goo", + Num: 5, }, }, "Some header of the issue\n\n\n\t PR: foobar#5 \n\t PR: rd/goo5 \n\t PR: test/#5 \n" + @@ -103,7 +103,7 @@ func TestAppendingPRsToDescription(t *testing.T) { "Append single PR to end of description", "something", []BasicPR{ - {org: "a", repo: "b", num: 100}, + {Org: "a", Repo: "b", Num: 100}, }, "something\n\nPR: a/b#100", }, @@ -111,11 +111,11 @@ func TestAppendingPRsToDescription(t *testing.T) { "Append multiple PR to end of description", "something", []BasicPR{ - {org: "a1", repo: "b", num: 100}, - {org: "a1", repo: "c", num: 100}, - {org: "a1", repo: "c", num: 101}, - {org: "b", repo: "b", num: 100}, - {org: "c", repo: "b", num: 100}, + {Org: "a1", Repo: "b", Num: 100}, + {Org: "a1", Repo: "c", Num: 100}, + {Org: "a1", Repo: "c", Num: 101}, + {Org: "b", Repo: "b", Num: 100}, + {Org: "c", Repo: "b", Num: 100}, }, "something\n\nPR: a1/b#100\nPR: a1/c#100\nPR: a1/c#101\nPR: b/b#100\nPR: c/b#100", }, @@ -123,13 +123,13 @@ func TestAppendingPRsToDescription(t *testing.T) { "Append multiple sorted PR to end of description and remove dups", "something", []BasicPR{ - {org: "a1", repo: "c", num: 101}, - {org: "a1", repo: "c", num: 100}, - {org: "c", repo: "b", num: 100}, - {org: "b", repo: "b", num: 100}, - {org: "a1", repo: "c", num: 101}, - {org: "a1", repo: "c", num: 101}, - {org: "a1", repo: "b", num: 100}, + {Org: "a1", Repo: "c", Num: 101}, + {Org: "a1", Repo: "c", Num: 100}, + {Org: "c", Repo: "b", Num: 100}, + {Org: "b", Repo: "b", Num: 100}, + {Org: "a1", Repo: "c", Num: 101}, + {Org: "a1", Repo: "c", Num: 101}, + {Org: "a1", Repo: "b", Num: 100}, }, "something\n\nPR: a1/b#100\nPR: a1/c#100\nPR: a1/c#101\nPR: b/b#100\nPR: c/b#100", }, diff --git a/bots-common/gitea_utils.go b/bots-common/gitea_utils.go index 49ae666..3a9aefb 100644 --- a/bots-common/gitea_utils.go +++ b/bots-common/gitea_utils.go @@ -60,6 +60,10 @@ type GiteaPRFetcher interface { GetAssociatedPRs(org, repo string, prNo int64) ([]*models.PullRequest, error) } +type GiteaMaintainershipInterface interface { + FetchMaintainershipFile(org, prjGit, branch string) ([]byte, error) +} + type Gitea interface { GetPullRequestAndReviews(org, project string, num int64) (*models.PullRequest, []*models.PullReview, error) GetPullNotifications(since *time.Time) ([]*models.NotificationThread, error) @@ -76,6 +80,7 @@ type Gitea interface { GetRecentPullRequests(org, repo string) ([]*models.PullRequest, error) GetRecentCommits(org, repo, branch string, commitNo int64) ([]*models.Commit, error) + GiteaMaintainershipInterface GiteaPRFetcher } @@ -416,7 +421,7 @@ func (gitea *GiteaTransport) GetAssociatedPRs(org, repo string, prNo int64) ([]* repository.NewRepoGetPullRequestParams(). WithOwner(org). WithRepo(repo). - WithIndex(prNo), + WithIndex(prNo), gitea.transport.DefaultAuthentication) if err != nil { @@ -426,7 +431,6 @@ func (gitea *GiteaTransport) GetAssociatedPRs(org, repo string, prNo int64) ([]* desc := prData.Payload.Body strings.Split(desc, "\n") - return nil, nil } diff --git a/workflow-pr/maintainership.go b/workflow-pr/maintainership.go index 76e54ab..f95a7e7 100644 --- a/workflow-pr/maintainership.go +++ b/workflow-pr/maintainership.go @@ -1,7 +1,10 @@ package main import ( + "bufio" "encoding/json" + "fmt" + "strings" "src.opensuse.org/autogits/common" "src.opensuse.org/autogits/common/gitea-generated/models" @@ -13,10 +16,6 @@ const ProjectKey = "" type MaintainershipMap map[string][]string -type GiteaMaintainershipInterface interface { - FetchMaintainershipFile(org, prjGit, branch string) ([]byte, error) -} - func parseMaintainershipData(data []byte) (MaintainershipMap, error) { maintainers := make(MaintainershipMap) if err := json.Unmarshal(data, &maintainers); err != nil { @@ -26,7 +25,7 @@ func parseMaintainershipData(data []byte) (MaintainershipMap, error) { return maintainers, nil } -func ProjectMaintainershipData(gitea GiteaMaintainershipInterface, org, prjGit, branch string) (MaintainershipMap, error) { +func ProjectMaintainershipData(gitea common.GiteaMaintainershipInterface, org, prjGit, branch string) (MaintainershipMap, error) { data, err := gitea.FetchMaintainershipFile(org, prjGit, branch) if err != nil || data == nil { return nil, err @@ -61,10 +60,50 @@ prjMaintainer: return pkgMaintainers } -func CheckIfMaintainersApproved(gitea GiteaPRInterface, config common.AutogitConfig, prjGitPRNumber int64) (bool, error) { - pr, reviews, _ := gitea.GetPullRequestAndReviews(config.Organization, config.GitProjectName, prjGitPRNumber) +type PRReviewInfo struct { + pr *models.PullRequest + reviews []*models.PullReview +} + +func fetchAllAssociatedPRs(gitea GiteaPRInterface, org, repo string, prNum int64) ([]PRReviewInfo, error) { + pr, reviews, err := gitea.GetPullRequestAndReviews(org, repo, prNum) + if err != nil { + return nil, err + } + + ret := make([]PRReviewInfo, 1, 2) + ret[0].pr = pr + ret[0].reviews = reviews + + return ret, 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) - maintainers, _ := parseMaintainershipData(data) + 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 + } + + 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) + } + } + + parseMaintainershipData(data) + return false, nil } diff --git a/workflow-pr/maintainership_test.go b/workflow-pr/maintainership_test.go index 45864c9..7ff521b 100644 --- a/workflow-pr/maintainership_test.go +++ b/workflow-pr/maintainership_test.go @@ -7,16 +7,18 @@ 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) { - allocateMaintainershipInterface := func(t *testing.T) *mock_main.MockGiteaMaintainershipInterface { + allocateMaintainershipInterface := func(t *testing.T) (*mock_common.MockGiteaMaintainershipInterface, *mock_main.MockGiteaPRInterface) { ctl := gomock.NewController(t) - mi := mock_main.NewMockGiteaMaintainershipInterface(ctl) + mi := mock_common.NewMockGiteaMaintainershipInterface(ctl) + pri := mock_main.NewMockGiteaPRInterface(ctl) - return mi + return mi, pri } config := common.AutogitConfig{ @@ -26,7 +28,7 @@ func TestMaintainership(t *testing.T) { } t.Run("No maintainer in empty package", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, nil) @@ -42,7 +44,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("No maintainer for empty projects", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, nil) @@ -62,7 +64,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Error in MaintainerListForPackage when remote has an error", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) err := errors.New("some error here") mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, err) @@ -79,7 +81,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Error in MaintainerListForProject when remote has an error", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) err := errors.New("some error here") mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return(nil, err) @@ -96,7 +98,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Multiple project maintainers", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -119,7 +121,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Single project maintainer", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -142,7 +144,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Invalid list of project maintainers", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -157,7 +159,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Invalid list of project maintainers", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -172,7 +174,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Multiple package maintainers", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -196,7 +198,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("No package maintainers and only project maintainer", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -221,7 +223,7 @@ func TestMaintainership(t *testing.T) { }) t.Run("Invalid list of package maintainers", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, _ := allocateMaintainershipInterface(t) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` { @@ -237,44 +239,44 @@ func TestMaintainership(t *testing.T) { }) t.Run("Maintainers not appoved", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, pri := allocateMaintainershipInterface(t) - mi.EXPECT().GetPullRequestAndReviews("foo", common.DefaultGitPrj, int64(10)).Return( + pri.EXPECT().GetPullRequestAndReviews("foo", common.DefaultGitPrj, int64(10)).Return( &models.PullRequest{ + Body: "", Index: 10, RequestedReviewers: []*models.User{}, }, []*models.PullReview{}, nil, ) - mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(`{}`), nil) + mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(`{"foo": ["bingo"]}`), nil) - approved, err := CheckIfMaintainersApproved(mi, config, 10) + approved, err := CheckIfMaintainersApproved(mi, pri, config, 10) if approved || err != nil { t.Error("Unexpected approved or err:", err) } }) t.Run("Maintainers approved", func(t *testing.T) { - mi := allocateMaintainershipInterface(t) + mi, pri := allocateMaintainershipInterface(t) - mi.EXPECT().GetPullRequestAndReviews("foo", common.DefaultGitPrj, int64(10)).Return( + pri.EXPECT().GetPullRequestAndReviews("foo", common.DefaultGitPrj, int64(10)).Return( &models.PullRequest{ + Body: "", Index: 10, RequestedReviewers: []*models.User{}, }, - []*models.PullReview{}, + []*models.PullReview{{Body: "ok", User: &models.User{UserName: "test"}, Stale: false, State: "approved"}}, nil, ) mi.EXPECT().FetchMaintainershipFile("foo", common.DefaultGitPrj, "bar").Return([]byte(` "": ["test"] `), nil) - approved, err := CheckIfMaintainersApproved(mi, config, 10) + approved, err := CheckIfMaintainersApproved(mi, pri, config, 10) if !approved || err != nil { t.Error("Unexpected disapproval or err:", err) } }) - - t.Parallel() } diff --git a/workflow-pr/pr.go b/workflow-pr/pr.go index 3349d4a..7d01996 100644 --- a/workflow-pr/pr.go +++ b/workflow-pr/pr.go @@ -5,11 +5,11 @@ import ( ) //go:generate mockgen -source=pr.go -destination=mock/pr.go -typed - type GiteaPRInterface interface { GetPullRequestAndReviews(org, pkg string, num int64) (*models.PullRequest, []*models.PullReview, error) - GetAssociatedPrjGitPR(org, repo string, id int) (*models.PullRequest, error) - GetAssociatedPRs(org, repo string, id int) ([]*models.PullRequest, error) + GetAssociatedPrjGitPR(org, repo string, id int64) (*models.PullRequest, error) + GetAssociatedPRs(org, repo string, id int64) ([]*models.PullRequest, error) } + diff --git a/workflow-pr/pr_test.go b/workflow-pr/pr_test.go index dd97f40..d5b7764 100644 --- a/workflow-pr/pr_test.go +++ b/workflow-pr/pr_test.go @@ -2,18 +2,19 @@ package main import ( "testing" - +/* "go.uber.org/mock/gomock" "src.opensuse.org/autogits/common/gitea-generated/models" mock_main "src.opensuse.org/workflow-pr/mock" + */ ) func TestPR(t *testing.T) { t.Run("Test simple PR to PrjGit checkout", func(t *testing.T) { - ctl := gomock.NewController(t) - pr := mock_main.NewMockGiteaPRInterface(ctl) + // ctl := gomock.NewController(t) + // pr := mock_main.NewMockGiteaPRInterface(ctl) - pr.EXPECT().GetPR("foo", "bar", 22).Return(&models.PullRequest{}, nil) + // pr.EXPECT().GetPR("foo", "bar", 22).Return(&models.PullRequest{}, nil) }) }