diff --git a/common/maintainership.go b/common/maintainership.go index 1165f31..038f28f 100644 --- a/common/maintainership.go +++ b/common/maintainership.go @@ -168,12 +168,32 @@ func (data *MaintainershipMap) IsApproved(pkg string, reviews []*models.PullRevi return true } - for _, review := range reviews { - if !review.Stale && review.State == ReviewStateApproved && slices.Contains(reviewers, review.User.UserName) { - LogDebug("Reviewed by", review.User.UserName) - return true + if !data.Config.ReviewRequired { + for _, review := range reviews { + if !review.Stale && review.State == ReviewStateApproved && slices.Contains(reviewers, review.User.UserName) { + LogDebug("Reviewed by", review.User.UserName) + return true + } } - + } else { + // all maintainers except submitter must approve + for _, m := range reviewers { + if m == submitter { + continue + } + approved := false + for _, review := range reviews { + if !review.Stale && review.State == ReviewStateApproved && review.User.UserName == m { + approved = true + break + } + } + if !approved { + LogDebug("Missing approval from maintainer:", m) + return false + } + } + return true } return false } diff --git a/common/pr.go b/common/pr.go index 9ccf31c..c805cf3 100644 --- a/common/pr.go +++ b/common/pr.go @@ -328,7 +328,7 @@ func (rs *PRSet) FindMissingAndExtraReviewers(maintainers MaintainershipData, id if len(rs.PRs) > 1 { noReviewPRCreators = append(noReviewPRCreators, rs.BotUser) } - if slices.Contains(noReviewPRCreators, pr.PR.User.UserName) || pr.Reviews.IsReviewedByOneOf(prjMaintainers...) { + if slices.Contains(noReviewPRCreators, pr.PR.User.UserName) || pr.Reviews.IsRejectedByOneOf(prjMaintainers...) || (pr.Reviews.IsReviewedByOneOf(prjMaintainers...) && !rs.Config.ReviewRequired) { LogDebug("Project already reviewed by a project maintainer, remove rest") // do not remove reviewers if they are also maintainers prjMaintainers = slices.DeleteFunc(prjMaintainers, func(m string) bool { return slices.Contains(missing, m) }) @@ -355,8 +355,8 @@ func (rs *PRSet) FindMissingAndExtraReviewers(maintainers MaintainershipData, id LogDebug("packakge maintainers:", Maintainers) missing = slices.Concat(configReviewers.Pkg, configReviewers.PkgOptional) - if slices.Contains(noReviewPkgPRCreators, pr.PR.User.UserName) || pr.Reviews.IsReviewedByOneOf(Maintainers...) { - // submitter is maintainer or already reviewed + if slices.Contains(noReviewPkgPRCreators, pr.PR.User.UserName) || pr.Reviews.IsRejectedByOneOf(Maintainers...) || (pr.Reviews.IsReviewedByOneOf(Maintainers...) && !rs.Config.ReviewRequired) { + // submitter is maintainer or already reviewed/rejected LogDebug("Package reviewed by maintainer (or subitter is maintainer), remove the rest of them") // do not remove reviewers if they are also maintainers Maintainers = slices.DeleteFunc(Maintainers, func(m string) bool { return slices.Contains(missing, m) }) diff --git a/common/reviews.go b/common/reviews.go index da03839..315773b 100644 --- a/common/reviews.go +++ b/common/reviews.go @@ -16,6 +16,8 @@ type ReviewInterface interface { HasPendingReviewBy(reviewer string) bool IsReviewedBy(reviewer string) bool IsReviewedByOneOf(reviewers ...string) bool + IsRejectedBy(reviewer string) bool + IsRejectedByOneOf(reviewers ...string) bool SetRequiredReviewers(reviewers []string) } @@ -230,7 +232,7 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool { for _, r := range r.Reviews { if r.User.UserName == reviewer && !r.Stale { switch r.State { - case ReviewStateApproved, ReviewStateRequestChanges: + case ReviewStateApproved: return true default: return false @@ -244,6 +246,37 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool { func (r *PRReviews) IsReviewedByOneOf(reviewers ...string) bool { for _, reviewer := range reviewers { if r.IsReviewedBy(reviewer) { + LogDebug(" -- IsReviewedByOneOf: matched ", reviewer) + return true + } + } + + return false +} + +func (r *PRReviews) IsRejectedBy(reviewer string) bool { + if r == nil { + return false + } + + for _, r := range r.Reviews { + if r.User.UserName == reviewer && !r.Stale { + switch r.State { + case ReviewStateRequestChanges: + return true + default: + return false + } + } + } + + return false +} + +func (r *PRReviews) IsRejectedByOneOf(reviewers ...string) bool { + for _, reviewer := range reviewers { + if r.IsRejectedBy(reviewer) { + LogDebug(" -- IsRejectedByOneOf: matched ", reviewer) return true } } diff --git a/integration/tests/workflow_pr_review_test.py b/integration/tests/workflow_pr_review_test.py index f3985c8..0c0587d 100644 --- a/integration/tests/workflow_pr_review_test.py +++ b/integration/tests/workflow_pr_review_test.py @@ -233,7 +233,6 @@ index 0000000..e69de29 @pytest.mark.t006 -@pytest.mark.xfail(reason="tbd flacky in ci") def test_006_maintainer_rejection_removes_other_requests(maintainer_env, ownerA_client, ownerBB_client): """ Test scenario: @@ -287,7 +286,6 @@ index 0000000..e69de29 @pytest.mark.t007 -@pytest.mark.xfail(reason="TBD troubleshoot") def test_007_review_required_needs_all_approvals(review_required_env, ownerA_client, ownerBB_client): """ Test scenario: