From 877e93c9bff3ef07e6489d304d991cd0f29d50990cc6e32d68964924fccfb920 Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Mon, 19 Jan 2026 14:28:51 +0100 Subject: [PATCH] pr: always require review, if configured Implement ReviewRequired option to workflow.config. This will always require a review by maintainer, unless no other maintainers are available. By default, ReviewRequired is false --- common/config.go | 7 ++-- common/maintainership.go | 11 +++++-- common/maintainership_test.go | 46 ++++++++++++++++++++++++-- common/pr.go | 10 ++++-- common/pr_test.go | 62 +++++++++++++++++++++++++++++++++-- workflow-pr/README.md | 4 +-- workflow-pr/pr_processor.go | 4 +-- 7 files changed, 128 insertions(+), 16 deletions(-) diff --git a/common/config.go b/common/config.go index dceccf2..dfb11d4 100644 --- a/common/config.go +++ b/common/config.go @@ -91,6 +91,7 @@ type AutogitConfig struct { NoProjectGitPR bool // do not automatically create project git PRs, just assign reviewers and assume somethign else creates the ProjectGit PR ManualMergeOnly bool // only merge with "Merge OK" comment by Project Maintainers and/or Package Maintainers and/or reviewers ManualMergeProject bool // require merge of ProjectGit PRs with "Merge OK" by ProjectMaintainers and/or reviewers + ReviewRequired bool // always require a maintainer review, even if maintainer submits it. Only ignored if no other package or project reviewers } type AutogitConfigs []*AutogitConfig @@ -292,9 +293,9 @@ func (config *AutogitConfig) GetRemoteBranch() string { } func (config *AutogitConfig) Label(label string) string { - if t, found := config.Labels[LabelKey(label)]; found { - return t - } + if t, found := config.Labels[LabelKey(label)]; found { + return t + } return label } diff --git a/common/maintainership.go b/common/maintainership.go index 39d2528..39165ae 100644 --- a/common/maintainership.go +++ b/common/maintainership.go @@ -25,6 +25,7 @@ const ProjectFileKey = "_project" type MaintainershipMap struct { Data map[string][]string IsDir bool + Config *AutogitConfig FetchPackage func(string) ([]byte, error) } @@ -39,7 +40,9 @@ func parseMaintainershipData(data []byte) (*MaintainershipMap, error) { return maintainers, nil } -func FetchProjectMaintainershipData(gitea GiteaMaintainershipReader, org, prjGit, branch string) (*MaintainershipMap, error) { +func FetchProjectMaintainershipData(gitea GiteaMaintainershipReader, config *AutogitConfig) (*MaintainershipMap, error) { + org, prjGit, branch := config.GetPrjGit() + data, _, err := gitea.FetchMaintainershipDirFile(org, prjGit, branch, ProjectFileKey) dir := true if err != nil || data == nil { @@ -61,6 +64,7 @@ func FetchProjectMaintainershipData(gitea GiteaMaintainershipReader, org, prjGit m, err := parseMaintainershipData(data) if m != nil { + m.Config = config m.IsDir = dir m.FetchPackage = func(pkg string) ([]byte, error) { data, _, err := gitea.FetchMaintainershipDirFile(org, prjGit, branch, pkg) @@ -149,7 +153,10 @@ func (data *MaintainershipMap) IsApproved(pkg string, reviews []*models.PullRevi } LogDebug("Looking for review by:", reviewers) - if slices.Contains(reviewers, submitter) { + slices.Sort(reviewers) + reviewers = slices.Compact(reviewers) + SubmitterIdxInReviewers := slices.Index(reviewers, submitter) + if SubmitterIdxInReviewers > -1 && (!data.Config.ReviewRequired || len(reviewers) == 1) { LogDebug("Submitter is maintainer. Approving.") return true } diff --git a/common/maintainership_test.go b/common/maintainership_test.go index e8c8573..eb121e7 100644 --- a/common/maintainership_test.go +++ b/common/maintainership_test.go @@ -13,10 +13,10 @@ import ( ) func TestMaintainership(t *testing.T) { - config := common.AutogitConfig{ + config := &common.AutogitConfig{ Branch: "bar", Organization: "foo", - GitProjectName: common.DefaultGitPrj, + GitProjectName: common.DefaultGitPrj + "#bar", } packageTests := []struct { @@ -141,7 +141,7 @@ func TestMaintainership(t *testing.T) { notFoundError := repository.NewRepoGetContentsNotFound() for _, test := range packageTests { runTests := func(t *testing.T, mi common.GiteaMaintainershipReader) { - maintainers, err := common.FetchProjectMaintainershipData(mi, config.Organization, config.GitProjectName, config.Branch) + maintainers, err := common.FetchProjectMaintainershipData(mi, config) if err != nil && !test.otherError { if test.maintainersFileErr == nil { t.Fatal("Unexpected error recieved", err) @@ -253,3 +253,43 @@ func TestMaintainershipFileWrite(t *testing.T) { }) } } + +func TestReviewRequired(t *testing.T) { + tests := []struct { + name string + maintainers []string + config *common.AutogitConfig + is_approved bool + }{ + { + name: "ReviewRequired=false", + maintainers: []string{"maintainer1", "maintainer2"}, + config: &common.AutogitConfig{ReviewRequired: false}, + is_approved: true, + }, + { + name: "ReviewRequired=true", + maintainers: []string{"maintainer1", "maintainer2"}, + config: &common.AutogitConfig{ReviewRequired: true}, + is_approved: false, + }, + { + name: "ReviewRequired=true", + maintainers: []string{"maintainer1"}, + config: &common.AutogitConfig{ReviewRequired: true}, + is_approved: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + m := &common.MaintainershipMap{ + Data: map[string][]string{"": test.maintainers}, + } + m.Config = test.config + if approved := m.IsApproved("", nil, "maintainer1", nil); approved != test.is_approved { + t.Error("Expected m.IsApproved()->", test.is_approved, "but didn't get it") + } + }) + } +} diff --git a/common/pr.go b/common/pr.go index b6d9e0d..7cae060 100644 --- a/common/pr.go +++ b/common/pr.go @@ -316,7 +316,10 @@ func (rs *PRSet) FindMissingAndExtraReviewers(maintainers MaintainershipData, id // only need project maintainer reviews if: // * not created by a bot and has other PRs, or // * not created by maintainer - noReviewPRCreators := prjMaintainers + noReviewPRCreators := []string{} + if !rs.Config.ReviewRequired { + noReviewPRCreators = prjMaintainers + } if len(rs.PRs) > 1 { noReviewPRCreators = append(noReviewPRCreators, rs.BotUser) } @@ -339,7 +342,10 @@ func (rs *PRSet) FindMissingAndExtraReviewers(maintainers MaintainershipData, id pkg := pr.PR.Base.Repo.Name pkgMaintainers := maintainers.ListPackageMaintainers(pkg, nil) Maintainers := slices.Concat(prjMaintainers, pkgMaintainers) - noReviewPkgPRCreators := pkgMaintainers + noReviewPkgPRCreators := []string{} + if !rs.Config.ReviewRequired { + noReviewPkgPRCreators = pkgMaintainers + } LogDebug("packakge maintainers:", Maintainers) diff --git a/common/pr_test.go b/common/pr_test.go index 67917c2..b01f0c0 100644 --- a/common/pr_test.go +++ b/common/pr_test.go @@ -833,7 +833,6 @@ func TestFindMissingAndExtraReviewers(t *testing.T) { []string{"autogits_obs_staging_bot", "user1"}, }, }, - { name: "Add reviewer if also maintainer where review by maintainer is not needed", prset: &common.PRSet{ @@ -1095,8 +1094,67 @@ func TestFindMissingAndExtraReviewers(t *testing.T) { expected_missing_reviewers: [][]string{{"pkgm2", "prj2"}}, expected_extra_reviewers: [][]string{{}, {"prj1"}}, }, + { + name: "Package maintainer submitter, AlwaysRequireReview=false", + prset: &common.PRSet{ + PRs: []*common.PRInfo{ + { + PR: &models.PullRequest{ + User: &models.User{UserName: "pkgmaintainer"}, + Base: &models.PRBranchInfo{Name: "main", Repo: &models.Repository{Name: "pkg", Owner: &models.User{UserName: "org"}}}, + }, + Reviews: &common.PRReviews{}, + }, + }, + Config: &common.AutogitConfig{ + GitProjectName: "prg/repo#main", + Organization: "org", + Branch: "main", + Reviewers: []string{}, + ReviewRequired: false, + }, + }, + maintainers: &common.MaintainershipMap{ + Data: map[string][]string{ + "pkg": {"pkgmaintainer", "pkgm1"}, + }, + }, + noAutoStaging: true, + expected_missing_reviewers: [][]string{ + {}, + }, + }, + { + name: "Package maintainer submitter, AlwaysRequireReview=true", + prset: &common.PRSet{ + PRs: []*common.PRInfo{ + { + PR: &models.PullRequest{ + User: &models.User{UserName: "pkgmaintainer"}, + Base: &models.PRBranchInfo{Name: "main", Repo: &models.Repository{Name: "pkg", Owner: &models.User{UserName: "org"}}}, + }, + Reviews: &common.PRReviews{}, + }, + }, + Config: &common.AutogitConfig{ + GitProjectName: "prg/repo#main", + Organization: "org", + Branch: "main", + Reviewers: []string{}, + ReviewRequired: true, + }, + }, + maintainers: &common.MaintainershipMap{ + Data: map[string][]string{ + "pkg": {"pkgmaintainer", "pkgm1"}, + }, + }, + noAutoStaging: true, + expected_missing_reviewers: [][]string{ + {"pkgm1"}, + }, + }, } - for _, test := range tests { t.Run(test.name, func(t *testing.T) { test.prset.HasAutoStaging = !test.noAutoStaging diff --git a/workflow-pr/README.md b/workflow-pr/README.md index 312b43f..7ead764 100644 --- a/workflow-pr/README.md +++ b/workflow-pr/README.md @@ -52,7 +52,7 @@ Config file | *GitProjectName* | Repository and branch where the ProjectGit lives. | no | string | **Format**: `org/project_repo#branch` | By default assumes `_ObsPrj` with default branch in the *Organization* | | *ManualMergeOnly* | Merges are permitted only upon receiving a "merge ok" comment from designated maintainers in the PkgGit PR. | no | bool | true, false | false | | *ManualMergeProject* | Merges are permitted only upon receiving a "merge ok" comment in the ProjectGit PR from project maintainers. | no | bool | true, false | false | -| *ReviewRequired* | (NOT IMPLEMENTED) If submitter is a maintainer, require review from another maintainer if available. | no | bool | true, false | false | +| *ReviewRequired* | If submitter is a maintainer, require review from another maintainer if available. | no | bool | true, false | false | | *NoProjectGitPR* | Do not create PrjGit PR, but still perform other tasks. | no | bool | true, false | false | | *Reviewers* | PrjGit reviewers. Additional review requests are triggered for associated PkgGit PRs. PrjGit PR is merged only when all reviews are complete. | no | array of strings | | `[]` | | *ReviewGroups* | If a group is specified in Reviewers, its members are listed here. | no | array of objects | | `[]` | @@ -160,4 +160,4 @@ Server configuration | Field | Type | Notes | | ----- | ----- | ----- | -| root | Array of string | Format **org/repo\#branch** | \ No newline at end of file +| root | Array of string | Format **org/repo\#branch** | diff --git a/workflow-pr/pr_processor.go b/workflow-pr/pr_processor.go index fe0b6cb..c1c5b85 100644 --- a/workflow-pr/pr_processor.go +++ b/workflow-pr/pr_processor.go @@ -500,7 +500,7 @@ func (pr *PRProcessor) Process(req *models.PullRequest) error { // make sure that prjgit is consistent and only submodules that are to be *updated* // reset anything that changed that is not part of the prset // package removals/additions are *not* counted here - org, repo, branch := config.GetPrjGit() + // TODO: this is broken... if pr, err := prset.GetPrjGitPR(); err == nil && false { common.LogDebug("Submodule parse begin") @@ -549,7 +549,7 @@ func (pr *PRProcessor) Process(req *models.PullRequest) error { } else { common.LogInfo("* No prjgit") } - maintainers, err := common.FetchProjectMaintainershipData(Gitea, org, repo, branch) + maintainers, err := common.FetchProjectMaintainershipData(Gitea, config) if err != nil { return err } -- 2.51.1