3 Commits

Author SHA256 Message Date
Andrii Nikitin
a335d04031 pr: enforce all maintainer approvals when ReviewRequired is true
Some checks failed
go-generate-check / go-generate-check (pull_request) Successful in 8s
Integration tests / t (pull_request) Has been cancelled
- Update MaintainershipMap.IsApproved to require approvals from all listed
  maintainers (excluding the submitter) when ReviewRequired is enabled.
- Split ReviewInterface logic to differentiate between approvals and
  rejections by adding IsRejectedBy and restricting IsReviewedBy to
  the APPROVED state.
- Fix premature removal of maintainer review requests in
  FindMissingAndExtraReviewers; ensure requests remain active if
  ReviewRequired is true and approvals are still missing.
- Ensure any maintainer rejection correctly triggers the cleanup of
  other redundant review requests.

Analysis Summary
The failures in test_006 and test_007 were caused by two primary logic bugs:
1. Conflated Review States: IsReviewedBy treated both "Approved" and "Request Changes" as the same state. This meant a rejection didn't properly trigger the "cleanup" logic, and an approval was indistinguishable from a rejection in several checks.
2. Greedy Reviewer Removal: The bot assumed that any maintainer approval meant all other maintainer requests could be removed. This contradicted the ReviewRequired flag, which (as seen in test_007) is intended to mandate multiple/all maintainer approvals before the PR is considered ready.
2026-03-03 15:52:25 +01:00
7ec663db27 rabbitmq: dial with timeout
Some checks failed
go-generate-check / go-generate-check (pull_request) Successful in 22s
Integration tests / t (pull_request) Failing after 5m50s
go-generate-check / go-generate-check (push) Successful in 14s
Integration tests / t (push) Successful in 4m26s
Hardcoded 10 second timeout on no connection instead of waiting
forever.
2026-03-03 09:28:44 +01:00
3b83ba96e4 pr: fix spam
Some checks failed
go-generate-check / go-generate-check (push) Successful in 12s
Integration tests / t (push) Failing after 7m46s
2026-03-02 16:55:57 +01:00
6 changed files with 70 additions and 15 deletions

View File

@@ -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
}

View File

@@ -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) })

View File

@@ -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
}
}

View File

@@ -92,10 +92,13 @@ func ConnectToExchangeForPublish(host, username, password string) {
auth = username + ":" + password + "@"
}
connection, err := rabbitmq.DialTLS("amqps://"+auth+host, &tls.Config{
ServerName: host,
connection, err := rabbitmq.DialConfig("amqps://"+auth+host, rabbitmq.Config{
Dial: rabbitmq.DefaultDial(10 * time.Second),
TLSClientConfig: &tls.Config{
ServerName: host,
},
})
failOnError(err, "Cannot connect to rabbit.opensuse.org")
failOnError(err, "Cannot connect to "+host)
defer connection.Close()
ch, err := connection.Channel()

View File

@@ -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:

View File

@@ -470,7 +470,8 @@ func (pr *PRProcessor) Process(req *models.PullRequest) error {
if pr.PR.State == "open" {
org, repo, idx := pr.PRComponents()
if prjGitPR.PR.HasMerged {
Gitea.AddComment(pr.PR, "This PR is merged via the associated Project PR.")
// TODO: use timeline here because this can spam if ManualMergePR fails
// Gitea.AddComment(pr.PR, "This PR is merged via the associated Project PR.")
err = Gitea.ManualMergePR(org, repo, idx, pr.PR.Head.Sha, false)
if _, ok := err.(*repository.RepoMergePullRequestConflict); !ok {
common.PanicOnError(err)