fix: github-bot multiple fixes on ReviewBy requirements (#4145)

aeddi · web-flow · commit 829bbbcb9fbe · 2025-04-15T08:23:26.000+02:00
This PR: - f99df53 Improves the `gnoweb` review rule to make it fit with [this description](#3238 (comment)): > a. If Alexis or Guilhem is the author of a PR related to gnoweb, the other is assigned as a reviewer. b. If someone else is the author of a PR related to gnoweb, both are assigned as reviewers, but only one review from one of them is required. - 7088bff Prevents [an error](#3238 (comment)) when the bot try to request a review from the PR author. - 24b4738 Adds a `RequestAction` mechanism similar to the `LabelAction` to the `ReviewByUser` and `ReviewByTeam` requirements.
diff --git a/contribs/github-bot/internal/config/config.go b/contribs/github-bot/internal/config/config.go
@@ -48,11 +48,11 @@ func Config(gh *client.GitHub) ([]AutomaticCheck, []ManualCheck) {
 			Then: r.And(
 				r.Or(
 					r.AuthorInTeam(gh, "tech-staff"),
-					r.ReviewByTeamMembers(gh, "tech-staff").WithDesiredState(utils.ReviewStateApproved),
+					r.ReviewByTeamMembers(gh, "tech-staff", r.RequestApply).WithDesiredState(utils.ReviewStateApproved),
 				),
 				r.Or(
 					r.AuthorInTeam(gh, "devrels"),
-					r.ReviewByTeamMembers(gh, "devrels").WithDesiredState(utils.ReviewStateApproved),
+					r.ReviewByTeamMembers(gh, "devrels", r.RequestApply).WithDesiredState(utils.ReviewStateApproved),
 				),
 			),
 		},
@@ -63,8 +63,26 @@ func Config(gh *client.GitHub) ([]AutomaticCheck, []ManualCheck) {
 				c.FileChanged(gh, "^gno.land/pkg/gnoweb/"),
 			),
 			Then: r.Or(
-				r.ReviewByUser(gh, "alexiscolin"),
-				r.ReviewByUser(gh, "gfanton"),
+				// If alexiscolin or gfanton is the author of the PR, the other must review it.
+				r.Or(
+					r.And(
+						r.Author("alexiscolin"),
+						r.ReviewByUser(gh, "gfanton", r.RequestApply).WithDesiredState(utils.ReviewStateApproved),
+					),
+					r.And(
+						r.Author("gfanton"),
+						r.ReviewByUser(gh, "alexiscolin", r.RequestApply).WithDesiredState(utils.ReviewStateApproved),
+					),
+				),
+				// If neither of them is the author of the PR, at least one of them must review it.
+				r.And(
+					r.Not(r.Author("alexiscolin")),
+					r.Not(r.Author("gfanton")),
+					r.Or(
+						r.ReviewByUser(gh, "alexiscolin", r.RequestApply).WithDesiredState(utils.ReviewStateApproved),
+						r.ReviewByUser(gh, "gfanton", r.RequestApply).WithDesiredState(utils.ReviewStateApproved),
+					),
+				),
 			),
 		},
 		{
@@ -81,16 +99,29 @@ func Config(gh *client.GitHub) ([]AutomaticCheck, []ManualCheck) {
 			Then: r.
 				If(r.Or(
 					r.ReviewByOrgMembers(gh).WithDesiredState(utils.ReviewStateApproved),
-					r.ReviewByTeamMembers(gh, "tech-staff"),
+					r.ReviewByTeamMembers(gh, "tech-staff", r.RequestIgnore),
 					r.Draft(),
 				)).
 				// Either there was a first approval from a member, and we
-				// assert that the label for triage-pending is removed...
-				Then(r.Not(r.Label(gh, "review/triage-pending", r.LabelRemove))).
-				// Or there was not, and we apply the triage pending label.
+				// assert that the label for triage-pending is removed and we
+				// request a review from the tech-staff team...
+				Then(
+					r.And(
+						r.Not(r.Label(gh, "review/triage-pending", r.LabelRemove)),
+						r.ReviewByTeamMembers(gh, "tech-staff", r.RequestApply),
+					),
+				).
+				// Or there was not, and we apply the triage-pending label
+				// and remove the request for review from the tech-staff team.
 				// The requirement should always fail, to mark the PR is not
 				// ready to be merged.
-				Else(r.And(r.Label(gh, "review/triage-pending", r.LabelApply), r.Never())),
+				Else(
+					r.And(
+						r.Label(gh, "review/triage-pending", r.LabelApply),
+						r.ReviewByTeamMembers(gh, "tech-staff", r.RequestRemove),
+						r.Never(), // Always fail the requirement.
+					),
+				),
 		},
 	}
 
diff --git a/contribs/github-bot/internal/requirements/reviewer.go b/contribs/github-bot/internal/requirements/reviewer.go
@@ -11,6 +11,18 @@ import (
 	"github.com/xlab/treeprint"
 )
 
+// RequestAction controls what to do about the review request.
+type RequestAction byte
+
+const (
+	// RequestApply will request a review from the user/team if not already requested.
+	RequestApply = iota
+	// RequestRemove will remove the review request from the user/team.
+	RequestRemove
+	// RequestIgnore always leaves the review request as it is.
+	RequestIgnore
+)
+
 // deduplicateReviews returns a list of reviews with at most 1 review per
 // author, where approval/changes requested reviews are preferred over comments
 // and later reviews are preferred over earlier ones.
@@ -60,6 +72,7 @@ type ReviewByUserRequirement struct {
 	gh           *client.GitHub
 	user         string
 	desiredState string
+	action       RequestAction
 }
 
 var _ Requirement = &ReviewByUserRequirement{}
@@ -88,8 +101,9 @@ func (r *ReviewByUserRequirement) IsSatisfied(pr *github.PullRequest, details tr
 	}
 	r.gh.Logger.Debugf("User %s has not reviewed PR %d yet", r.user, pr.GetNumber())
 
-	// If not a dry run, make the user a reviewer if he's not already.
-	if !r.gh.DryRun {
+	// If not a dry run, change the review request according to the action.
+	if !r.gh.DryRun && r.action != RequestIgnore {
+		// Check if this user is already requested for review.
 		requested := false
 		reviewers, err := r.gh.ListPRReviewers(pr.GetNumber())
 		if err != nil {
@@ -104,20 +118,44 @@ func (r *ReviewByUserRequirement) IsSatisfied(pr *github.PullRequest, details tr
 			}
 		}
 
-		if requested {
-			r.gh.Logger.Debugf("Review of user %s already requested on PR %d", r.user, pr.GetNumber())
-		} else {
-			r.gh.Logger.Debugf("Requesting review from user %s on PR %d", r.user, pr.GetNumber())
-			if _, _, err := r.gh.Client.PullRequests.RequestReviewers(
-				r.gh.Ctx,
-				r.gh.Owner,
-				r.gh.Repo,
-				pr.GetNumber(),
-				github.ReviewersRequest{
-					Reviewers: []string{r.user},
-				},
-			); err != nil {
-				r.gh.Logger.Errorf("Unable to request review from user %s on PR %d: %v", r.user, pr.GetNumber(), err)
+		switch r.action {
+		case RequestApply:
+			switch {
+			case requested:
+				r.gh.Logger.Debugf("Review of user %s already requested on PR %d", r.user, pr.GetNumber())
+			case r.user == pr.GetUser().GetLogin():
+				r.gh.Logger.Debugf("Review of user %s is not requested on PR %d because he's the author", r.user, pr.GetNumber())
+			default:
+				r.gh.Logger.Debugf("Requesting review from user %s on PR %d", r.user, pr.GetNumber())
+				if _, _, err := r.gh.Client.PullRequests.RequestReviewers(
+					r.gh.Ctx,
+					r.gh.Owner,
+					r.gh.Repo,
+					pr.GetNumber(),
+					github.ReviewersRequest{
+						Reviewers: []string{r.user},
+					},
+				); err != nil {
+					r.gh.Logger.Errorf("Unable to request review from user %s on PR %d: %v", r.user, pr.GetNumber(), err)
+				}
+			}
+		case RequestRemove:
+			switch {
+			case !requested:
+				r.gh.Logger.Debugf("Review of user %s already not requested on PR %d", r.user, pr.GetNumber())
+			default:
+				r.gh.Logger.Debugf("Removing review request from user %s on PR %d", r.user, pr.GetNumber())
+				if _, err := r.gh.Client.PullRequests.RemoveReviewers(
+					r.gh.Ctx,
+					r.gh.Owner,
+					r.gh.Repo,
+					pr.GetNumber(),
+					github.ReviewersRequest{
+						Reviewers: []string{r.user},
+					},
+				); err != nil {
+					r.gh.Logger.Errorf("Unable to remove review request from user %s on PR %d: %v", r.user, pr.GetNumber(), err)
+				}
 			}
 		}
 	}
@@ -138,8 +176,12 @@ func (r *ReviewByUserRequirement) WithDesiredState(s utils.ReviewState) *ReviewB
 }
 
 // ReviewByUser asserts that the PR has been reviewed by the given user.
-func ReviewByUser(gh *client.GitHub, user string) *ReviewByUserRequirement {
-	return &ReviewByUserRequirement{gh: gh, user: user}
+func ReviewByUser(gh *client.GitHub, user string, action RequestAction) *ReviewByUserRequirement {
+	return &ReviewByUserRequirement{
+		gh:     gh,
+		user:   user,
+		action: action,
+	}
 }
 
 // ReviewByTeamMembersRequirement asserts that count members of the given team
@@ -150,6 +192,7 @@ type ReviewByTeamMembersRequirement struct {
 	team         string
 	count        uint
 	desiredState string
+	action       RequestAction
 }
 
 var _ Requirement = &ReviewByTeamMembersRequirement{}
@@ -177,7 +220,8 @@ func (r *ReviewByTeamMembersRequirement) IsSatisfied(pr *github.PullRequest, det
 
 	// If not a dry run, request a team review if no member has reviewed yet,
 	// and the team review has not been requested.
-	if !r.gh.DryRun {
+	if !r.gh.DryRun && r.action != RequestIgnore {
+		// Check if the team or any of its members are already requested for review.
 		var teamRequested bool
 		var usersRequested []string
 
@@ -210,24 +254,45 @@ func (r *ReviewByTeamMembersRequirement) IsSatisfied(pr *github.PullRequest, det
 			}
 		}
 
-		switch {
-		case teamRequested:
-			r.gh.Logger.Debugf("Review of team %s already requested on PR %d", r.team, pr.GetNumber())
-		case len(usersRequested) > 0:
-			r.gh.Logger.Debugf("Members %v of team %s already requested on (or reviewed) PR %d",
-				usersRequested, r.team, pr.GetNumber())
-		default:
-			r.gh.Logger.Debugf("Requesting review from team %s on PR %d", r.team, pr.GetNumber())
-			if _, _, err := r.gh.Client.PullRequests.RequestReviewers(
-				r.gh.Ctx,
-				r.gh.Owner,
-				r.gh.Repo,
-				pr.GetNumber(),
-				github.ReviewersRequest{
-					TeamReviewers: []string{r.team},
-				},
-			); err != nil {
-				r.gh.Logger.Errorf("Unable to request review from team %s on PR %d: %v", r.team, pr.GetNumber(), err)
+		switch r.action {
+		case RequestApply:
+			switch {
+			case teamRequested:
+				r.gh.Logger.Debugf("Review of team %s already requested on PR %d", r.team, pr.GetNumber())
+			case len(usersRequested) > 0:
+				r.gh.Logger.Debugf("Members %v of team %s already requested on (or reviewed) PR %d",
+					usersRequested, r.team, pr.GetNumber())
+			default:
+				r.gh.Logger.Debugf("Requesting review from team %s on PR %d", r.team, pr.GetNumber())
+				if _, _, err := r.gh.Client.PullRequests.RequestReviewers(
+					r.gh.Ctx,
+					r.gh.Owner,
+					r.gh.Repo,
+					pr.GetNumber(),
+					github.ReviewersRequest{
+						TeamReviewers: []string{r.team},
+					},
+				); err != nil {
+					r.gh.Logger.Errorf("Unable to request review from team %s on PR %d: %v", r.team, pr.GetNumber(), err)
+				}
+			}
+		case RequestRemove:
+			switch {
+			case !teamRequested:
+				r.gh.Logger.Debugf("Review of team %s already not requested on PR %d", r.team, pr.GetNumber())
+			default:
+				r.gh.Logger.Debugf("Removing review request from team %s on PR %d", r.team, pr.GetNumber())
+				if _, err := r.gh.Client.PullRequests.RemoveReviewers(
+					r.gh.Ctx,
+					r.gh.Owner,
+					r.gh.Repo,
+					pr.GetNumber(),
+					github.ReviewersRequest{
+						TeamReviewers: []string{r.team},
+					},
+				); err != nil {
+					r.gh.Logger.Errorf("Unable to remove review request from team %s on PR %d: %v", r.team, pr.GetNumber(), err)
+				}
 			}
 		}
 	}
@@ -283,11 +348,12 @@ func (r *ReviewByTeamMembersRequirement) WithDesiredState(s utils.ReviewState) *
 //
 // The number of required reviews, or the state of the reviews (e.g., to filter
 // only for approval reviews) can be modified using WithCount and WithDesiredState.
-func ReviewByTeamMembers(gh *client.GitHub, team string) *ReviewByTeamMembersRequirement {
+func ReviewByTeamMembers(gh *client.GitHub, team string, action RequestAction) *ReviewByTeamMembersRequirement {
 	return &ReviewByTeamMembersRequirement{
-		gh:    gh,
-		team:  team,
-		count: 1,
+		gh:     gh,
+		team:   team,
+		count:  1,
+		action: action,
 	}
 }
 
diff --git a/contribs/github-bot/internal/requirements/reviewer_test.go b/contribs/github-bot/internal/requirements/reviewer_test.go
