From b7f9f745e2d0b36bd29548a4b0169e9d10b3fe6e Mon Sep 17 00:00:00 2001 From: Morgan Date: Thu, 23 Jan 2025 15:20:10 +0100 Subject: [PATCH] feat(github-bot): avoid triage-pending if there is any tech-staff review (#3574) After a first day of experimentation, we determined we don't want the triage-pending label on PRs that have already received a review from a tech-staff member. Some additional changes: - Split between `ApprovalBy` and `ReviewBy` in reviewer.go, to distinguish between the requirements that only work with an approval and those that are more generic. - Don't request a team review if a member of the team already reviewed or is requested for review. (The bot was nagging a bit on some PRs because of this) --- contribs/github-bot/internal/config/config.go | 13 +- .../internal/requirements/reviewer.go | 223 +++++++++++++----- .../internal/requirements/reviewer_test.go | 111 +++++---- .../github-bot/internal/utils/github_const.go | 20 ++ 4 files changed, 269 insertions(+), 98 deletions(-) diff --git a/contribs/github-bot/internal/config/config.go b/contribs/github-bot/internal/config/config.go index 83219c04d1d..43a505ad15a 100644 --- a/contribs/github-bot/internal/config/config.go +++ b/contribs/github-bot/internal/config/config.go @@ -4,6 +4,7 @@ import ( "github.com/gnolang/gno/contribs/github-bot/internal/client" c "github.com/gnolang/gno/contribs/github-bot/internal/conditions" r "github.com/gnolang/gno/contribs/github-bot/internal/requirements" + "github.com/gnolang/gno/contribs/github-bot/internal/utils" ) type Teams []string @@ -41,11 +42,11 @@ func Config(gh *client.GitHub) ([]AutomaticCheck, []ManualCheck) { Then: r.And( r.Or( r.AuthorInTeam(gh, "tech-staff"), - r.ReviewByTeamMembers(gh, "tech-staff", 1), + r.ReviewByTeamMembers(gh, "tech-staff").WithDesiredState(utils.ReviewStateApproved), ), r.Or( r.AuthorInTeam(gh, "devrels"), - r.ReviewByTeamMembers(gh, "devrels", 1), + r.ReviewByTeamMembers(gh, "devrels").WithDesiredState(utils.ReviewStateApproved), ), ), }, @@ -55,10 +56,14 @@ func Config(gh *client.GitHub) ([]AutomaticCheck, []ManualCheck) { Then: r.Never(), }, { - Description: "Pending initial approval by a review team member (and label matches review triage state)", + Description: "Pending initial approval by a review team member, or review from tech-staff", If: c.Not(c.AuthorInTeam(gh, "tech-staff")), Then: r. - If(r.Or(r.ReviewByOrgMembers(gh, 1), r.Draft())). + If(r.Or( + r.ReviewByOrgMembers(gh).WithDesiredState(utils.ReviewStateApproved), + r.ReviewByTeamMembers(gh, "tech-staff"), + 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))). diff --git a/contribs/github-bot/internal/requirements/reviewer.go b/contribs/github-bot/internal/requirements/reviewer.go index 3c0d4f98510..3e91c394fb7 100644 --- a/contribs/github-bot/internal/requirements/reviewer.go +++ b/contribs/github-bot/internal/requirements/reviewer.go @@ -2,6 +2,7 @@ package requirements import ( "fmt" + "slices" "github.com/gnolang/gno/contribs/github-bot/internal/client" "github.com/gnolang/gno/contribs/github-bot/internal/utils" @@ -10,18 +11,22 @@ import ( "github.com/xlab/treeprint" ) -// Reviewer Requirement. -type reviewByUser struct { - gh *client.GitHub - user string +// ReviewByUserRequirement asserts that there is a review by the given user, +// and if given that the review matches the desiredState. +type ReviewByUserRequirement struct { + gh *client.GitHub + user string + desiredState string } -const approvedState = "APPROVED" +var _ Requirement = &ReviewByUserRequirement{} -var _ Requirement = &reviewByUser{} - -func (r *reviewByUser) IsSatisfied(pr *github.PullRequest, details treeprint.Tree) bool { - detail := fmt.Sprintf("This user approved pull request: %s", r.user) +// IsSatisfied implements [Requirement]. +func (r *ReviewByUserRequirement) IsSatisfied(pr *github.PullRequest, details treeprint.Tree) bool { + detail := fmt.Sprintf("This user reviewed pull request: %s", r.user) + if r.desiredState != "" { + detail += fmt.Sprintf(" (with state %q)", r.desiredState) + } // If not a dry run, make the user a reviewer if he's not already. if !r.gh.DryRun { @@ -67,7 +72,8 @@ func (r *reviewByUser) IsSatisfied(pr *github.PullRequest, details treeprint.Tre for _, review := range reviews { if review.GetUser().GetLogin() == r.user { r.gh.Logger.Debugf("User %s already reviewed PR %d with state %s", r.user, pr.GetNumber(), review.GetState()) - return utils.AddStatusNode(review.GetState() == approvedState, detail, details) + result := r.desiredState == "" || review.GetState() == r.desiredState + return utils.AddStatusNode(result, detail, details) } } r.gh.Logger.Debugf("User %s has not reviewed PR %d yet", r.user, pr.GetNumber()) @@ -75,25 +81,54 @@ func (r *reviewByUser) IsSatisfied(pr *github.PullRequest, details treeprint.Tre return utils.AddStatusNode(false, detail, details) } -func ReviewByUser(gh *client.GitHub, user string) Requirement { - return &reviewByUser{gh, user} +// WithDesiredState asserts that the review by the given user should also be +// of the given ReviewState. +// +// If an empty string is passed, then all reviews are counted. This is the default. +func (r *ReviewByUserRequirement) WithDesiredState(s utils.ReviewState) *ReviewByUserRequirement { + if s != "" && !s.Valid() { + panic(fmt.Sprintf("invalid state: %q", s)) + } + r.desiredState = string(s) + return r } -// Reviewer Requirement. -type reviewByTeamMembers struct { - gh *client.GitHub - team string - count uint +// 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} } -var _ Requirement = &reviewByTeamMembers{} +// ReviewByTeamMembersRequirement asserts that count members of the given team +// have reviewed the PR. Additionally, using desiredState, it may be required +// that the PR reviews be of that state. +type ReviewByTeamMembersRequirement struct { + gh *client.GitHub + team string + count uint + desiredState string +} -func (r *reviewByTeamMembers) IsSatisfied(pr *github.PullRequest, details treeprint.Tree) bool { - detail := fmt.Sprintf("At least %d user(s) of the team %s approved pull request", r.count, r.team) +var _ Requirement = &ReviewByTeamMembersRequirement{} - // If not a dry run, make the user a reviewer if he's not already. +// IsSatisfied implements [Requirement]. +func (r *ReviewByTeamMembersRequirement) IsSatisfied(pr *github.PullRequest, details treeprint.Tree) bool { + detail := fmt.Sprintf("At least %d user(s) of the team %s reviewed pull request", r.count, r.team) + if r.desiredState != "" { + detail += fmt.Sprintf("(with state %q)", r.desiredState) + } + + teamMembers, err := r.gh.ListTeamMembers(r.team) + if err != nil { + r.gh.Logger.Errorf(err.Error()) + return utils.AddStatusNode(false, detail, details) + } + + // 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 { - requested := false + var teamRequested bool + var usersRequested []string + reviewers, err := r.gh.ListPRReviewers(pr.GetNumber()) if err != nil { r.gh.Logger.Errorf("unable to check if team %s review is already requested: %v", r.team, err) @@ -102,14 +137,28 @@ func (r *reviewByTeamMembers) IsSatisfied(pr *github.PullRequest, details treepr for _, team := range reviewers.Teams { if team.GetSlug() == r.team { - requested = true + teamRequested = true break } } - if requested { + if !teamRequested { + for _, user := range reviewers.Users { + if slices.ContainsFunc(teamMembers, func(memb *github.User) bool { + return memb.GetID() == user.GetID() + }) { + usersRequested = append(usersRequested, user.GetLogin()) + } + } + } + + switch { + case teamRequested: r.gh.Logger.Debugf("Review of team %s already requested on PR %d", r.team, pr.GetNumber()) - } else { + 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, @@ -125,72 +174,138 @@ func (r *reviewByTeamMembers) IsSatisfied(pr *github.PullRequest, details treepr } } - // Check how many members of this team already approved this PR. - approved := uint(0) + // Check how many members of this team already reviewed this PR. + reviewCount := uint(0) reviews, err := r.gh.ListPRReviews(pr.GetNumber()) if err != nil { - r.gh.Logger.Errorf("unable to check if a member of team %s already approved this PR: %v", r.team, err) + r.gh.Logger.Errorf("unable to check if a member of team %s already reviewed this PR: %v", r.team, err) return utils.AddStatusNode(false, detail, details) } - teamMembers, err := r.gh.ListTeamMembers(r.team) - if err != nil { - r.gh.Logger.Errorf(err.Error()) - return utils.AddStatusNode(false, detail, details) + stateStr := "" + if r.desiredState != "" { + stateStr = fmt.Sprintf("%q ", r.desiredState) } - for _, review := range reviews { for _, member := range teamMembers { if review.GetUser().GetLogin() == member.GetLogin() { - if review.GetState() == approvedState { - approved += 1 + if desired := r.desiredState; desired == "" || desired == review.GetState() { + reviewCount += 1 } - r.gh.Logger.Debugf("Member %s from team %s already reviewed PR %d with state %s (%d/%d required approval(s))", member.GetLogin(), r.team, pr.GetNumber(), review.GetState(), approved, r.count) + r.gh.Logger.Debugf( + "Member %s from team %s already reviewed PR %d with state %s (%d/%d required %sreview(s))", + member.GetLogin(), r.team, pr.GetNumber(), review.GetState(), reviewCount, r.count, stateStr, + ) } } } - return utils.AddStatusNode(approved >= r.count, detail, details) + return utils.AddStatusNode(reviewCount >= r.count, detail, details) } -func ReviewByTeamMembers(gh *client.GitHub, team string, count uint) Requirement { - return &reviewByTeamMembers{gh, team, count} +// WithCount specifies the number of required reviews. +// By default, this is 1. +func (r *ReviewByTeamMembersRequirement) WithCount(n uint) *ReviewByTeamMembersRequirement { + if n < 1 { + panic("number of required reviews should be at least 1") + } + r.count = n + return r } -type reviewByOrgMembers struct { - gh *client.GitHub - count uint +// WithDesiredState asserts that the reviews should also be of the given ReviewState. +// +// If an empty string is passed, then all reviews are counted. This is the default. +func (r *ReviewByTeamMembersRequirement) WithDesiredState(s utils.ReviewState) *ReviewByTeamMembersRequirement { + if s != "" && !s.Valid() { + panic(fmt.Sprintf("invalid state: %q", s)) + } + r.desiredState = string(s) + return r } -var _ Requirement = &reviewByOrgMembers{} +// ReviewByTeamMembers specifies that the given pull request should receive at +// least one review from a member of the given team. +// +// 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 { + return &ReviewByTeamMembersRequirement{ + gh: gh, + team: team, + count: 1, + } +} -func (r *reviewByOrgMembers) IsSatisfied(pr *github.PullRequest, details treeprint.Tree) bool { - detail := fmt.Sprintf("At least %d user(s) of the organization approved the pull request", r.count) +// ReviewByOrgMembersRequirement asserts that the given PR has been reviewed by +// at least count members of the given organization, filtering for PR reviews +// with state desiredState. +type ReviewByOrgMembersRequirement struct { + gh *client.GitHub + count uint + desiredState string +} - // Check how many members of this team already approved this PR. - approved := uint(0) +var _ Requirement = &ReviewByOrgMembersRequirement{} + +// IsSatisfied implements [Requirement]. +func (r *ReviewByOrgMembersRequirement) IsSatisfied(pr *github.PullRequest, details treeprint.Tree) bool { + detail := fmt.Sprintf("At least %d user(s) of the organization reviewed the pull request", r.count) + if r.desiredState != "" { + detail += fmt.Sprintf(" (with state %q)", r.desiredState) + } + + // Check how many members of this team already reviewed this PR. + reviewed := uint(0) reviews, err := r.gh.ListPRReviews(pr.GetNumber()) if err != nil { r.gh.Logger.Errorf("unable to check number of reviews on this PR: %v", err) return utils.AddStatusNode(false, detail, details) } + stateStr := "" + if r.desiredState != "" { + stateStr = fmt.Sprintf("%q ", r.desiredState) + } for _, review := range reviews { if review.GetAuthorAssociation() == "MEMBER" { - if review.GetState() == approvedState { - approved++ + if r.desiredState == "" || review.GetState() == r.desiredState { + reviewed++ } r.gh.Logger.Debugf( - "Member %s already reviewed PR %d with state %s (%d/%d required approval(s))", + "Member %s already reviewed PR %d with state %s (%d/%d required %sreviews)", review.GetUser().GetLogin(), pr.GetNumber(), review.GetState(), - approved, r.count, + reviewed, r.count, stateStr, ) } } - return utils.AddStatusNode(approved >= r.count, detail, details) + return utils.AddStatusNode(reviewed >= r.count, detail, details) +} + +// WithCount specifies the number of required reviews. +// By default, this is 1. +func (r *ReviewByOrgMembersRequirement) WithCount(n uint) *ReviewByOrgMembersRequirement { + if n < 1 { + panic("number of required reviews should be at least 1") + } + r.count = n + return r +} + +// WithDesiredState asserts that the reviews should also be of the given ReviewState. +// +// If an empty string is passed, then all reviews are counted. This is the default. +func (r *ReviewByOrgMembersRequirement) WithDesiredState(s utils.ReviewState) *ReviewByOrgMembersRequirement { + if s != "" && !s.Valid() { + panic(fmt.Sprintf("invalid state: %q", s)) + } + r.desiredState = string(s) + return r } -func ReviewByOrgMembers(gh *client.GitHub, count uint) Requirement { - return &reviewByOrgMembers{gh, count} +// ReviewByOrgMembers asserts that at least 1 member of the organization +// reviewed this PR. +func ReviewByOrgMembers(gh *client.GitHub) *ReviewByOrgMembersRequirement { + return &ReviewByOrgMembersRequirement{gh: gh, count: 1} } diff --git a/contribs/github-bot/internal/requirements/reviewer_test.go b/contribs/github-bot/internal/requirements/reviewer_test.go index 98f68384d8f..235dca14034 100644 --- a/contribs/github-bot/internal/requirements/reviewer_test.go +++ b/contribs/github-bot/internal/requirements/reviewer_test.go @@ -36,7 +36,7 @@ func TestReviewByUser(t *testing.T) { State: github.String("APPROVED"), }, { User: &github.User{Login: github.String("anotherOne")}, - State: github.String("REQUEST_CHANGES"), + State: github.String("CHANGES_REQUESTED"), }, } @@ -87,7 +87,7 @@ func TestReviewByUser(t *testing.T) { pr := &github.PullRequest{} details := treeprint.New() - requirement := ReviewByUser(gh, testCase.user) + requirement := ReviewByUser(gh, testCase.user).WithDesiredState(utils.ReviewStateApproved) assert.Equal(t, requirement.IsSatisfied(pr, details), testCase.isSatisfied, fmt.Sprintf("requirement should have a satisfied status: %t", testCase.isSatisfied)) assert.True(t, utils.TestLastNodeStatus(t, testCase.isSatisfied, details), fmt.Sprintf("requirement details should have a status: %t", testCase.isSatisfied)) @@ -99,13 +99,25 @@ func TestReviewByUser(t *testing.T) { func TestReviewByTeamMembers(t *testing.T) { t.Parallel() - reviewers := github.Reviewers{ - Teams: []*github.Team{ - {Slug: github.String("team1")}, - {Slug: github.String("team2")}, - {Slug: github.String("team3")}, - }, - } + var ( + reviewers = github.Reviewers{ + Teams: []*github.Team{ + {Slug: github.String("team1")}, + {Slug: github.String("team2")}, + {Slug: github.String("team3")}, + }, + } + noReviewers = github.Reviewers{} + userReviewers = github.Reviewers{ + Users: []*github.User{ + {Login: github.String("user1")}, + {Login: github.String("user2")}, + {Login: github.String("user3")}, + {Login: github.String("user4")}, + {Login: github.String("user5")}, + }, + } + ) members := map[string][]*github.User{ "team1": { @@ -136,26 +148,37 @@ func TestReviewByTeamMembers(t *testing.T) { State: github.String("APPROVED"), }, { User: &github.User{Login: github.String("user4")}, - State: github.String("REQUEST_CHANGES"), + State: github.String("CHANGES_REQUESTED"), }, { User: &github.User{Login: github.String("user5")}, - State: github.String("REQUEST_CHANGES"), + State: github.String("CHANGES_REQUESTED"), }, } + const ( + notSatisfied = 0 + satisfied = 1 + withRequest = 2 + ) + for _, testCase := range []struct { - name string - team string - count uint - isSatisfied bool - testRequest bool + name string + team string + count uint + state utils.ReviewState + reviewers github.Reviewers + expectedResult byte }{ - {"3/3 team members approved;", "team1", 3, true, false}, - {"1/1 team member approved", "team2", 1, true, false}, - {"1/2 team member approved", "team2", 2, false, false}, - {"0/1 team member approved", "team3", 1, false, false}, - {"0/1 team member approved with request", "team3", 1, false, true}, - {"team doesn't exist with request", "team4", 1, false, true}, + {"3/3 team members approved", "team1", 3, utils.ReviewStateApproved, reviewers, satisfied}, + {"3/3 team members approved (with user reviewers)", "team1", 3, utils.ReviewStateApproved, userReviewers, satisfied}, + {"1/1 team member approved", "team2", 1, utils.ReviewStateApproved, reviewers, satisfied}, + {"1/2 team member approved", "team2", 2, utils.ReviewStateApproved, reviewers, notSatisfied}, + {"0/1 team member approved", "team3", 1, utils.ReviewStateApproved, reviewers, notSatisfied}, + {"0/1 team member approved with request", "team3", 1, utils.ReviewStateApproved, noReviewers, notSatisfied | withRequest}, + {"team doesn't exist with request", "team4", 1, utils.ReviewStateApproved, noReviewers, notSatisfied | withRequest}, + {"3/3 team members reviewed", "team2", 3, "", reviewers, satisfied}, + {"2/2 team members rejected", "team2", 2, utils.ReviewStateChangesRequested, reviewers, satisfied}, + {"1/3 team members approved", "team2", 3, utils.ReviewStateApproved, reviewers, notSatisfied}, } { t.Run(testCase.name, func(t *testing.T) { t.Parallel() @@ -170,11 +193,7 @@ func TestReviewByTeamMembers(t *testing.T) { }, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { if firstRequest { - if testCase.testRequest { - w.Write(mock.MustMarshal(github.Reviewers{})) - } else { - w.Write(mock.MustMarshal(reviewers)) - } + w.Write(mock.MustMarshal(testCase.reviewers)) firstRequest = false } else { requested = true @@ -205,11 +224,18 @@ func TestReviewByTeamMembers(t *testing.T) { pr := &github.PullRequest{} details := treeprint.New() - requirement := ReviewByTeamMembers(gh, testCase.team, testCase.count) + requirement := ReviewByTeamMembers(gh, testCase.team). + WithCount(testCase.count). + WithDesiredState(testCase.state) - assert.Equal(t, requirement.IsSatisfied(pr, details), testCase.isSatisfied, fmt.Sprintf("requirement should have a satisfied status: %t", testCase.isSatisfied)) - assert.True(t, utils.TestLastNodeStatus(t, testCase.isSatisfied, details), fmt.Sprintf("requirement details should have a status: %t", testCase.isSatisfied)) - assert.Equal(t, testCase.testRequest, requested, fmt.Sprintf("requirement should have requested to create item: %t", testCase.testRequest)) + expSatisfied := testCase.expectedResult&satisfied != 0 + expRequested := testCase.expectedResult&withRequest > 0 + assert.Equal(t, expSatisfied, requirement.IsSatisfied(pr, details), + "requirement should have a satisfied status: %t", expSatisfied) + assert.True(t, utils.TestLastNodeStatus(t, expSatisfied, details), + "requirement details should have a status: %t", expSatisfied) + assert.Equal(t, expRequested, requested, + "requirement should have requested to create item: %t", expRequested) }) } } @@ -232,23 +258,26 @@ func TestReviewByOrgMembers(t *testing.T) { AuthorAssociation: github.String("MEMBER"), }, { User: &github.User{Login: github.String("user4")}, - State: github.String("REQUEST_CHANGES"), + State: github.String("CHANGES_REQUESTED"), AuthorAssociation: github.String("MEMBER"), }, { User: &github.User{Login: github.String("user5")}, - State: github.String("REQUEST_CHANGES"), + State: github.String("CHANGES_REQUESTED"), AuthorAssociation: github.String("NONE"), }, } for _, testCase := range []struct { - name string - count uint - isSatisfied bool + name string + count uint + desiredState utils.ReviewState + isSatisfied bool }{ - {"2/3 org members approved", 3, false}, - {"2/2 org members approved", 2, true}, - {"2/1 org members approved", 1, true}, + {"2/3 org members approved", 3, utils.ReviewStateApproved, false}, + {"2/2 org members approved", 2, utils.ReviewStateApproved, true}, + {"2/1 org members approved", 1, utils.ReviewStateApproved, true}, + {"3/3 org members reviewed", 3, "", true}, + {"3/4 org members reviewed", 4, "", false}, } { t.Run(testCase.name, func(t *testing.T) { t.Parallel() @@ -271,7 +300,9 @@ func TestReviewByOrgMembers(t *testing.T) { pr := &github.PullRequest{} details := treeprint.New() - requirement := ReviewByOrgMembers(gh, testCase.count) + requirement := ReviewByOrgMembers(gh). + WithCount(testCase.count). + WithDesiredState(testCase.desiredState) assert.Equal(t, requirement.IsSatisfied(pr, details), testCase.isSatisfied, fmt.Sprintf("requirement should have a satisfied status: %t", testCase.isSatisfied)) assert.True(t, utils.TestLastNodeStatus(t, testCase.isSatisfied, details), fmt.Sprintf("requirement details should have a status: %t", testCase.isSatisfied)) diff --git a/contribs/github-bot/internal/utils/github_const.go b/contribs/github-bot/internal/utils/github_const.go index f030d9365f7..423fcb2a4b0 100644 --- a/contribs/github-bot/internal/utils/github_const.go +++ b/contribs/github-bot/internal/utils/github_const.go @@ -13,3 +13,23 @@ const ( PRStateOpen = "open" PRStateClosed = "closed" ) + +// ReviewState is the state of a PR review. See: +// https://docs.github.com/en/graphql/reference/enums#pullrequestreviewstate +type ReviewState string + +// Possible values of ReviewState. +const ( + ReviewStateApproved ReviewState = "APPROVED" + ReviewStateChangesRequested ReviewState = "CHANGES_REQUESTED" + ReviewStateCommented ReviewState = "COMMENTED" +) + +// Valid determines whether the ReviewState is one of the known ReviewStates. +func (r ReviewState) Valid() bool { + switch r { + case ReviewStateApproved, ReviewStateChangesRequested, ReviewStateCommented: + return true + } + return false +}