Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bot: Fix cloud reviewer assignments #212

Merged
merged 5 commits into from
Feb 8, 2024
Merged

bot: Fix cloud reviewer assignments #212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3dfab4
bot: Update isInternal check to include cloud and core maps
jimbishopp Jan 8, 2024
c50647b
add back removed test
jimbishopp Jan 10, 2024
e3ace4f
Add preferredOnly flag
jimbishopp Feb 7, 2024
4f00e65
fix json tag
jimbishopp Feb 7, 2024
51a9251
fix comment
jimbishopp Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion bot/internal/bot/assign_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import (
// TestBackportReviewers checks if backport reviewers are correctly assigned.
func TestBackportReviewers(t *testing.T) {
r, err := review.New(&review.Config{
CodeReviewers: map[string]review.Reviewer{},
CoreReviewers: map[string]review.Reviewer{},
CloudReviewers: map[string]review.Reviewer{},
CodeReviewersOmit: map[string]bool{},
DocsReviewers: map[string]review.Reviewer{},
DocsReviewersOmit: map[string]bool{},
Expand Down
5 changes: 2 additions & 3 deletions bot/internal/bot/backport_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ func TestFindBranches(t *testing.T) {
func TestBackport(t *testing.T) {
buildTestBot := func(github Client) (*Bot, context.Context) {
r, _ := review.New(&review.Config{
CodeReviewers: map[string]review.Reviewer{"dev": review.Reviewer{
Team: "core",
}},
CoreReviewers: map[string]review.Reviewer{"dev": review.Reviewer{}},
CloudReviewers: map[string]review.Reviewer{},
CodeReviewersOmit: map[string]bool{},
DocsReviewers: map[string]review.Reviewer{},
DocsReviewersOmit: map[string]bool{},
Expand Down
3 changes: 2 additions & 1 deletion bot/internal/bot/bloat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ func createFileWithSize(t *testing.T, path string, sizeInMB int64) {
func TestBloatCheck(t *testing.T) {
r, err := review.New(&review.Config{
Admins: []string{"admin1", "admin2"},
CodeReviewers: make(map[string]review.Reviewer),
CoreReviewers: make(map[string]review.Reviewer),
CloudReviewers: make(map[string]review.Reviewer),
CodeReviewersOmit: make(map[string]bool),
DocsReviewers: make(map[string]review.Reviewer),
DocsReviewersOmit: make(map[string]bool),
Expand Down
5 changes: 3 additions & 2 deletions bot/internal/bot/bot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,14 @@ func TestIsInternal(t *testing.T) {
}
rc := &review.Config{
Admins: []string{},
CodeReviewers: make(map[string]review.Reviewer),
CoreReviewers: make(map[string]review.Reviewer),
CloudReviewers: make(map[string]review.Reviewer),
CodeReviewersOmit: map[string]bool{},
DocsReviewers: make(map[string]review.Reviewer),
DocsReviewersOmit: make(map[string]bool),
}
for _, cr := range test.codeReviewers {
rc.CodeReviewers[cr] = review.Reviewer{}
rc.CoreReviewers[cr] = review.Reviewer{}
}
for _, dr := range test.docsReviewers {
rc.DocsReviewers[dr] = review.Reviewer{}
Expand Down
3 changes: 2 additions & 1 deletion bot/internal/bot/check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,13 @@ func TestDismissUnnecessaryReviewers(t *testing.T) {
t.Run(test.desc, func(t *testing.T) {
a, err := review.New(&review.Config{
Admins: []string{"admin1"},
CodeReviewers: map[string]review.Reviewer{
CoreReviewers: map[string]review.Reviewer{
"user1": {},
"user2": {},
"user3": {},
"user4": {},
},
CloudReviewers: make(map[string]review.Reviewer),
CodeReviewersOmit: make(map[string]bool),
DocsReviewers: make(map[string]review.Reviewer),
DocsReviewersOmit: make(map[string]bool),
Expand Down
3 changes: 2 additions & 1 deletion bot/internal/bot/flake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ import (
func TestSkipFlakes(t *testing.T) {
r, err := review.New(&review.Config{
Admins: []string{"admin1", "admin2"},
CodeReviewers: make(map[string]review.Reviewer),
CoreReviewers: make(map[string]review.Reviewer),
CloudReviewers: make(map[string]review.Reviewer),
CodeReviewersOmit: make(map[string]bool),
DocsReviewers: make(map[string]review.Reviewer),
DocsReviewersOmit: make(map[string]bool),
Expand Down
3 changes: 2 additions & 1 deletion bot/internal/bot/skip_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import (
func TestSkipItems(t *testing.T) {
r, err := review.New(&review.Config{
Admins: []string{"admin1", "admin2"},
CodeReviewers: make(map[string]review.Reviewer),
CoreReviewers: make(map[string]review.Reviewer),
CloudReviewers: make(map[string]review.Reviewer),
CodeReviewersOmit: make(map[string]bool),
DocsReviewers: make(map[string]review.Reviewer),
DocsReviewersOmit: make(map[string]bool),
Expand Down
150 changes: 60 additions & 90 deletions bot/internal/review/review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package review

import (
"encoding/json"
"fmt"
"log"
"math/rand"
"sort"
Expand Down Expand Up @@ -93,15 +92,10 @@ func isAllowedRobot(author string) bool {

// Reviewer is a code reviewer.
type Reviewer struct {
// Team the reviewer belongs to. This field is set when loading the configuration file
// based on whether the section being loaded is either CoreReviewers or CloudReviewers.
// DocsReviewers is automatically set to Core.
//
// Deprecated: we should remove the need for this field and detect by environment now
// that the teams have been split into their own sections in the configuration file.
Team string `json:"team"`
// Owner is true if the reviewer is a code or docs owner (required for all reviews).
Owner bool `json:"owner"`
// PreferredOnly is true if the reviewer should only be included in preferred reviewer paths.
PreferredOnly bool `json:"preferredOnly,omitempty"`
// PreferredReviewerFor contains a list of file paths that this reviewer
// should be selected to review.
PreferredReviewerFor []string `json:"preferredReviewerFor,omitempty"`
Expand All @@ -118,16 +112,11 @@ type Config struct {
// operations.
Rand Rand

// CodeReviewers and CodeReviewersOmit is a map of code reviews and code
// reviewers to omit. CodeReviewers is set to either CoreReviewers
// or CloudReviewers depending on the repository when the configuration
// file is loaded if CodeReviewers is empty.
CodeReviewers map[string]Reviewer `json:"codeReviewers"`
CodeReviewersOmit map[string]bool `json:"codeReviewersOmit"`
// CodeReviewersOmit is a map of code reviews and code reviewers to omit.
CodeReviewersOmit map[string]bool `json:"codeReviewersOmit"`

// CoreReviewers and CloudReviewers defines reviewers for the respositories
// owned by the core and cloud teams. One of these is assigned to CodeReviewers
// depending on the repository when the configuration file is loaded.
// owned by the core and cloud teams.
CoreReviewers map[string]Reviewer `json:"coreReviewers"`
CloudReviewers map[string]Reviewer `json:"cloudReviewers"`

Expand All @@ -149,9 +138,14 @@ func (c *Config) CheckAndSetDefaults() error {
c.Rand = rand.New(rand.NewSource(time.Now().UnixNano()))
}

if c.CodeReviewers == nil {
return trace.BadParameter("missing parameter CodeReviewers")
if c.CoreReviewers == nil {
return trace.BadParameter("missing parameter CoreReviewers")
}

if c.CloudReviewers == nil {
return trace.BadParameter("missing parameter CloudReviewers")
}

if c.CodeReviewersOmit == nil {
return trace.BadParameter("missing parameter CodeReviewersOmit")
}
Expand Down Expand Up @@ -182,23 +176,6 @@ func FromString(e *env.Environment, reviewers string) (*Assignments, error) {
return nil, trace.Wrap(err)
}

// hacky way of allowing the same reviewer to be defined in
// both cloud and core repos without having to change the
// bot in a large number of places.
if len(c.CodeReviewers) == 0 { // allow this change to deploy before updating the config file
switch e.RepoOwnerTeam() {
case env.CloudTeam:
c.CodeReviewers = setReviewerTeam(env.CloudTeam, c.CloudReviewers)
case env.CoreTeam:
c.CodeReviewers = setReviewerTeam(env.CoreTeam, c.CoreReviewers)
default:
return nil, trace.BadParameter("unable to detect code reviewers due to invalid team: %s", e.RepoOwnerTeam())
}
}

// team for all docs reviewers should be set to Core
c.DocsReviewers = setReviewerTeam(env.CoreTeam, c.DocsReviewers)

r, err := New(&c)
if err != nil {
return nil, trace.Wrap(err)
Expand All @@ -225,9 +202,10 @@ func (r *Assignments) IsInternal(author string) bool {
return true
}

_, code := r.c.CodeReviewers[author]
_, core := r.c.CoreReviewers[author]
_, cloud := r.c.CloudReviewers[author]
_, docs := r.c.DocsReviewers[author]
return code || docs
return core || cloud || docs
}

// Get will return a list of code reviewers for a given author.
Expand Down Expand Up @@ -257,19 +235,30 @@ func (r *Assignments) Get(e *env.Environment, changes env.Changes, files []githu
return reviewers
}

func (r *Assignments) repoReviewers(repo string) map[string]Reviewer {
switch repo {
case env.TeleportRepo, env.TeleportERepo:
return r.c.CoreReviewers
case env.CloudRepo:
return r.c.CloudReviewers
}
return map[string]Reviewer{}
}

func (r *Assignments) getReleaseReviewers() []string {
return r.c.ReleaseReviewers
}

func (r *Assignments) getDocsReviewers(e *env.Environment, files []github.PullRequestFile) []string {
// See if any code reviewers are designated preferred reviewers for one of
// the changed docs files. If so, add them as docs reviewers.
a, b := getReviewerSets(e.Author, "Core", r.c.CodeReviewers, r.c.CodeReviewersOmit)
prefCodeReviewers := r.getAllPreferredReviewers(append(a, b...), files)
repoReviewers := r.repoReviewers(e.Repository)
a, b := getReviewerSets(e.Author, repoReviewers, r.c.CodeReviewersOmit)
prefCodeReviewers := r.getAllPreferredReviewers(repoReviewers, append(a, b...), files)

// Get the docs reviewer pool, which does not depend on the files
// changed by a pull request.
docsA, docsB := getReviewerSets(e.Author, "Core", r.c.DocsReviewers, r.c.DocsReviewersOmit)
docsA, docsB := getReviewerSets(e.Author, r.c.DocsReviewers, r.c.DocsReviewersOmit)
reviewers := append(prefCodeReviewers, append(docsA, docsB...)...)

// If no docs reviewers were assigned, assign admin reviews.
Expand All @@ -281,6 +270,8 @@ func (r *Assignments) getDocsReviewers(e *env.Environment, files []github.PullRe
}

func (r *Assignments) getCodeReviewers(e *env.Environment, files []github.PullRequestFile) []string {
reviewers := r.repoReviewers(e.Repository)

// Obtain full sets of reviewers.
setA, setB := r.getCodeReviewerSets(e)

Expand All @@ -290,17 +281,21 @@ func (r *Assignments) getCodeReviewers(e *env.Environment, files []github.PullRe
sort.Strings(setB)

// See if there are preferred reviewers for the changeset.
preferredSetA := r.getPreferredReviewers(setA, files)
preferredSetB := r.getPreferredReviewers(setB, files)
preferredSetA := r.getPreferredReviewers(reviewers, setA, files)
preferredSetB := r.getPreferredReviewers(reviewers, setB, files)

// All preferred reviewers should be requested reviews. If there are none,
// pick from the overall set at random.
resultingSetA := preferredSetA
if len(resultingSetA) == 0 {
// Only include reviewers from setA whose preferredOnly field is false.
setA = filterPreferredOnly(reviewers, setA, false)
resultingSetA = append(resultingSetA, setA[r.c.Rand.Intn(len(setA))])
}
resultingSetB := preferredSetB
if len(resultingSetB) == 0 {
// Only include reviewers from setB whose preferredOnly field is false.
setB = filterPreferredOnly(reviewers, setB, false)
resultingSetB = append(resultingSetB, setB[r.c.Rand.Intn(len(setB))])
}

Expand All @@ -310,11 +305,11 @@ func (r *Assignments) getCodeReviewers(e *env.Environment, files []github.PullRe
// getPreferredReviewers returns a list of reviewers that would be preferrable
// to review the provided changeset. Returns at most one preferred reviewer per
// file path.
func (r *Assignments) getPreferredReviewers(set []string, files []github.PullRequestFile) (preferredReviewers []string) {
func (r *Assignments) getPreferredReviewers(teamReviewers map[string]Reviewer, set []string, files []github.PullRequestFile) (preferredReviewers []string) {
// To avoid assigning too many reviewers iterate over paths that we have
// preferred reviewers for and see if any of them are among the changeset.
coveredPaths := make(map[string]struct{})
for path, reviewers := range r.getPreferredReviewersMap(set) {
for path, reviewers := range r.getPreferredReviewersMap(teamReviewers, set) {
if _, ok := coveredPaths[path]; ok {
continue
}
Expand All @@ -323,7 +318,7 @@ func (r *Assignments) getPreferredReviewers(set []string, files []github.PullReq
reviewer := reviewers[r.c.Rand.Intn(len(reviewers))]
log.Printf("Picking %v as preferred reviewer for %v which matches %v.", reviewer, file.Name, path)
preferredReviewers = append(preferredReviewers, reviewer)
for _, path := range r.c.CodeReviewers[reviewer].PreferredReviewerFor {
for _, path := range teamReviewers[reviewer].PreferredReviewerFor {
coveredPaths[path] = struct{}{}
}
break
Expand All @@ -336,14 +331,14 @@ func (r *Assignments) getPreferredReviewers(set []string, files []github.PullReq
// getAllPreferredReviewers returns a list of reviewers that would be
// preferrable to review the provided changeset. Includes all preferred
// reviewers for each file path in the changeset.
func (r *Assignments) getAllPreferredReviewers(set []string, files []github.PullRequestFile) (preferredReviewers []string) {
func (r *Assignments) getAllPreferredReviewers(reviewers map[string]Reviewer, set []string, files []github.PullRequestFile) (preferredReviewers []string) {
// Check each key in the preferred reviewer map, which is a file path
// that reviewers are assigned to. For any file names in the changeset
// that begin with that file path, add the reviewers for that pile path
// to the set of preferred reviewers. Look up each reviewer in a map to
// avoid duplication.
assigned := make(map[string]struct{})
for path, reviewers := range r.getPreferredReviewersMap(set) {
for path, reviewers := range r.getPreferredReviewersMap(reviewers, set) {
for _, file := range files {
if !strings.HasPrefix(file.Name, path) {
continue
Expand All @@ -361,10 +356,10 @@ func (r *Assignments) getAllPreferredReviewers(set []string, files []github.Pull
}

// getPreferredReviewersMap builds a map of preferred reviewers for file paths.
func (r *Assignments) getPreferredReviewersMap(set []string) map[string][]string {
func (r *Assignments) getPreferredReviewersMap(reviewers map[string]Reviewer, set []string) map[string][]string {
m := make(map[string][]string)
for _, name := range set {
if reviewer, ok := r.c.CodeReviewers[name]; ok {
if reviewer, ok := reviewers[name]; ok {
for _, path := range reviewer.PreferredReviewerFor {
m[path] = append(m[path], name)
}
Expand Down Expand Up @@ -392,24 +387,12 @@ func (r *Assignments) getAdminReviewers(author string) []string {
func (r *Assignments) getCodeReviewerSets(e *env.Environment) ([]string, []string) {
// Internal non-Core contributors get assigned from the admin reviewer set.
// Admins will review, triage, and re-assign.
v, ok := r.c.CodeReviewers[e.Author]
if !ok || v.Team == env.InternalTeam {
if !r.IsInternal(e.Author) {
Comment on lines -395 to +390
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change resolves the issue where a core engineer not defined in the cloud reviewer map submits a PR to the cloud repo but requires admin approval. Calling IsInternal now checks core, docs, and cloud maps to determine if the author is internal.

reviewers := r.getAdminReviewers(e.Author)
n := len(reviewers) / 2
return reviewers[:n], reviewers[n:]
}

team := v.Team

// Teams do their own internal reviews
switch e.Repository {
case env.TeleportRepo:
team = env.CoreTeam
case env.CloudRepo:
team = env.CloudTeam
}

return getReviewerSets(e.Author, team, r.c.CodeReviewers, r.c.CodeReviewersOmit)
return getReviewerSets(e.Author, r.repoReviewers(e.Repository), r.c.CodeReviewersOmit)
}

// CheckExternal requires two admins have approved.
Expand Down Expand Up @@ -509,18 +492,7 @@ func (r *Assignments) checkInternalDocsReviews(e *env.Environment, reviews []git
// checkInternalCodeReviews checks whether code review requirements are satisfied
// for a PR authored by an internal employee
func (r *Assignments) checkInternalCodeReviews(e *env.Environment, changes env.Changes, reviews []github.Review) error {
// Teams do their own internal reviews
var team string
switch e.Repository {
case env.TeleportRepo, env.TeleportERepo:
team = env.CoreTeam
case env.CloudRepo:
team = env.CloudTeam
default:
return trace.Wrap(fmt.Errorf("unsupported repository: %s", e.Repository))
}

setA, setB := getReviewerSets(e.Author, team, r.c.CodeReviewers, r.c.CodeReviewersOmit)
setA, setB := getReviewerSets(e.Author, r.repoReviewers(e.Repository), r.c.CodeReviewersOmit)

// PRs can be approved if you either have multiple code owners that approve
// or code owner and code reviewer. An exception is for PRs that
Expand Down Expand Up @@ -551,15 +523,11 @@ func (r *Assignments) GetAdminCheckers(author string) []string {
return reviewers
}

func getReviewerSets(author string, team string, reviewers map[string]Reviewer, reviewersOmit map[string]bool) ([]string, []string) {
func getReviewerSets(author string, reviewers map[string]Reviewer, reviewersOmit map[string]bool) ([]string, []string) {
var setA []string
var setB []string

for k, v := range reviewers {
// Only assign within a team.
if v.Team != team {
continue
}
// Skip over reviewers that are marked as omit.
if _, ok := reviewersOmit[k]; ok {
continue
Expand Down Expand Up @@ -612,17 +580,19 @@ func reviewsByAuthor(reviews []github.Review) map[string]string {
return m
}

// setReviewerTeam sets the Team field in each Reviewer of the reviewers map.
//
// NOTE: This is a hack so the team field doesn't need to be included in the config file.
// Eventually we should remove the team field and use the environment since the config file
// now separates cloud and core engineers.
func setReviewerTeam(team string, reviewers map[string]Reviewer) map[string]Reviewer {
for name, r := range reviewers {
r.Team = team
reviewers[name] = r
// filterPreferredOnly returns all names in set whose preferredOnly field is set to the preferredOnly parameter.
func filterPreferredOnly(reviewers map[string]Reviewer, set []string, preferredOnly bool) []string {
filtered := make([]string, 0, len(set))
for _, name := range set {
reviewer, ok := reviewers[name]
if !ok {
continue
}
if reviewer.PreferredOnly == preferredOnly {
filtered = append(filtered, name)
}
}
return reviewers
return filtered
}

const (
Expand Down
Loading
Loading