diff --git a/bot/internal/bot/assign_test.go b/bot/internal/bot/assign_test.go index b1129ca0..d231e9f7 100644 --- a/bot/internal/bot/assign_test.go +++ b/bot/internal/bot/assign_test.go @@ -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{}, diff --git a/bot/internal/bot/backport_test.go b/bot/internal/bot/backport_test.go index 0cdf0e08..1d3fc911 100644 --- a/bot/internal/bot/backport_test.go +++ b/bot/internal/bot/backport_test.go @@ -43,15 +43,15 @@ 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", - }}, + c := &review.Config{ + 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{}, Admins: []string{}, - }) + } + r, _ := review.New(c) return &Bot{ c: &Config{ diff --git a/bot/internal/bot/bloat_test.go b/bot/internal/bot/bloat_test.go index 91f78ab3..9c6f18a9 100644 --- a/bot/internal/bot/bloat_test.go +++ b/bot/internal/bot/bloat_test.go @@ -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), diff --git a/bot/internal/bot/bot_test.go b/bot/internal/bot/bot_test.go index 8f3ef7ab..dbbf8e57 100644 --- a/bot/internal/bot/bot_test.go +++ b/bot/internal/bot/bot_test.go @@ -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{} diff --git a/bot/internal/bot/check_test.go b/bot/internal/bot/check_test.go index 5ac90bf1..d59491f8 100644 --- a/bot/internal/bot/check_test.go +++ b/bot/internal/bot/check_test.go @@ -119,18 +119,20 @@ func TestDismissUnnecessaryReviewers(t *testing.T) { }, } { t.Run(test.desc, func(t *testing.T) { - a, err := review.New(&review.Config{ + c := &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), - }) + } + a, err := review.New(c) require.NoError(t, err) b := &Bot{ diff --git a/bot/internal/bot/flake_test.go b/bot/internal/bot/flake_test.go index e3391805..845fa8dd 100644 --- a/bot/internal/bot/flake_test.go +++ b/bot/internal/bot/flake_test.go @@ -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), diff --git a/bot/internal/bot/skip_test.go b/bot/internal/bot/skip_test.go index 17d2b806..e856e57d 100644 --- a/bot/internal/bot/skip_test.go +++ b/bot/internal/bot/skip_test.go @@ -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), diff --git a/bot/internal/review/review.go b/bot/internal/review/review.go index b77554ae..ebef5607 100644 --- a/bot/internal/review/review.go +++ b/bot/internal/review/review.go @@ -18,7 +18,6 @@ package review import ( "encoding/json" - "fmt" "log" "math/rand" "sort" @@ -86,13 +85,6 @@ 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"` // PreferredReviewerFor contains a list of file paths that this reviewer @@ -115,8 +107,8 @@ type Config struct { // 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"` + //CodeReviewers map[string]Reviewer `json:"codeReviewers"` + 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 @@ -142,9 +134,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") } @@ -175,23 +172,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) @@ -218,9 +198,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. @@ -250,6 +231,16 @@ 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 } @@ -257,12 +248,13 @@ func (r *Assignments) getReleaseReviewers() []string { 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. @@ -274,6 +266,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) @@ -283,8 +277,8 @@ 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. @@ -303,11 +297,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 } @@ -316,7 +310,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 @@ -329,14 +323,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 @@ -354,10 +348,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) } @@ -385,24 +379,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) { 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. @@ -502,18 +484,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 @@ -544,15 +515,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 @@ -605,19 +572,6 @@ 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 - } - return reviewers -} - const ( // Commented is a code review where the reviewer has left comments only. Commented = "COMMENTED" diff --git a/bot/internal/review/review_test.go b/bot/internal/review/review_test.go index 45b8e17d..adf52dc7 100644 --- a/bot/internal/review/review_test.go +++ b/bot/internal/review/review_test.go @@ -17,14 +17,13 @@ limitations under the License. package review import ( + "slices" "sort" "testing" - "github.com/stretchr/testify/require" - "golang.org/x/exp/slices" - "github.com/gravitational/shared-workflows/bot/internal/env" "github.com/gravitational/shared-workflows/bot/internal/github" + "github.com/stretchr/testify/require" ) // TestIsInternal checks if docs and code reviewers show up as internal. @@ -36,21 +35,21 @@ func TestIsInternal(t *testing.T) { expect bool }{ { - desc: "code-is-internal", + desc: "core-is-internal", assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: false}, - "4": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: false}, + "4": {Owner: false}, }, CodeReviewersOmit: map[string]bool{}, // Docs. DocsReviewers: map[string]Reviewer{ - "5": {Team: "Core", Owner: true}, - "6": {Team: "Core", Owner: true}, + "5": {Owner: true}, + "6": {Owner: true}, }, DocsReviewersOmit: map[string]bool{}, // Admins. @@ -63,22 +62,46 @@ func TestIsInternal(t *testing.T) { author: "1", expect: true, }, + { + desc: "cloud-is-internal", + assignments: &Assignments{ + c: &Config{ + // Code. + CloudReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: false}, + "4": {Owner: false}, + }, + CodeReviewersOmit: map[string]bool{}, + DocsReviewers: map[string]Reviewer{}, + DocsReviewersOmit: map[string]bool{}, + // Admins. + Admins: []string{ + "1", + "2", + }, + }, + }, + author: "1", + expect: true, + }, { desc: "docs-is-internal", assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: false}, - "4": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: false}, + "4": {Owner: false}, }, CodeReviewersOmit: map[string]bool{}, // Docs. DocsReviewers: map[string]Reviewer{ - "5": {Team: "Core", Owner: true}, - "6": {Team: "Core", Owner: true}, + "5": {Owner: true}, + "6": {Owner: true}, }, DocsReviewersOmit: map[string]bool{}, // Admins. @@ -96,17 +119,17 @@ func TestIsInternal(t *testing.T) { assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: false}, - "4": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: false}, + "4": {Owner: false}, }, CodeReviewersOmit: map[string]bool{}, // Docs. DocsReviewers: map[string]Reviewer{ - "5": {Team: "Core", Owner: true}, - "6": {Team: "Core", Owner: true}, + "5": {Owner: true}, + "6": {Owner: true}, }, DocsReviewersOmit: map[string]bool{}, // Admins. @@ -183,12 +206,13 @@ func TestGetCodeReviewers(t *testing.T) { assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: false}, - "4": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: false}, + "4": {Owner: false}, }, + CloudReviewers: map[string]Reviewer{}, CodeReviewersOmit: map[string]bool{}, // Admins. Admins: []string{ @@ -207,13 +231,14 @@ func TestGetCodeReviewers(t *testing.T) { assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: false}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: false}, + "4": {Owner: false}, + "5": {Owner: false}, }, + CloudReviewers: map[string]Reviewer{}, CodeReviewersOmit: map[string]bool{ "3": true, }, @@ -229,79 +254,20 @@ func TestGetCodeReviewers(t *testing.T) { setA: []string{"1", "2"}, setB: []string{"4"}, }, - { - desc: "internal-gets-defaults", - assignments: &Assignments{ - c: &Config{ - // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: false}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Internal"}, - }, - CodeReviewersOmit: map[string]bool{}, - // Admins. - Admins: []string{ - "1", - "2", - }, - }, - }, - repository: "teleport", - author: "5", - setA: []string{"1"}, - setB: []string{"2"}, - }, - { - desc: "cloud-gets-core-reviewers", - assignments: &Assignments{ - c: &Config{ - // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: true}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Core", Owner: false}, - "6": {Team: "Core", Owner: false}, - "7": {Team: "Internal", Owner: false}, - "8": {Team: "Cloud", Owner: false}, - "9": {Team: "Cloud", Owner: false}, - }, - CodeReviewersOmit: map[string]bool{ - "6": true, - }, - // Docs. - DocsReviewers: map[string]Reviewer{}, - DocsReviewersOmit: map[string]bool{}, - // Admins. - Admins: []string{ - "1", - "2", - }, - }, - }, - repository: "teleport", - author: "8", - setA: []string{"1", "2", "3"}, - setB: []string{"4", "5"}, - }, { desc: "normal", assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: true}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Core", Owner: false}, - "6": {Team: "Core", Owner: false}, - "7": {Team: "Internal", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: true}, + "4": {Owner: false}, + "5": {Owner: false}, + "6": {Owner: false}, }, + CloudReviewers: map[string]Reviewer{}, CodeReviewersOmit: map[string]bool{ "6": true, }, @@ -325,15 +291,15 @@ func TestGetCodeReviewers(t *testing.T) { assignments: &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: true}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Core", Owner: false}, - "6": {Team: "Core", Owner: false}, - "7": {Team: "Internal", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: true}, + "4": {Owner: false}, + "5": {Owner: false}, + "6": {Owner: false}, }, + CloudReviewers: map[string]Reviewer{}, CodeReviewersOmit: map[string]bool{ "6": true, }, @@ -356,12 +322,13 @@ func TestGetCodeReviewers(t *testing.T) { desc: "docs reviewers submitting code changes are treated as internal authors", assignments: &Assignments{ c: &Config{ - CodeReviewers: map[string]Reviewer{ - "code-1": {Team: "Core", Owner: true}, - "code-2": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "code-1": {Owner: true}, + "code-2": {Owner: false}, }, + CloudReviewers: map[string]Reviewer{}, DocsReviewers: map[string]Reviewer{ - "docs-1": {Team: "Core", Owner: true}, + "docs-1": {Owner: true}, }, Admins: []string{"code-1", "code-2"}, }, @@ -375,15 +342,16 @@ func TestGetCodeReviewers(t *testing.T) { desc: "admins can be omitted from code reviews", assignments: &Assignments{ c: &Config{ - CodeReviewers: map[string]Reviewer{ - "code-1": {Team: "Core", Owner: true}, - "code-2": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "code-1": {Owner: true}, + "code-2": {Owner: false}, }, Admins: []string{ "code-1", "code-2", "code-3", }, + CloudReviewers: map[string]Reviewer{}, CodeReviewersOmit: map[string]bool{ "code-1": true, }, @@ -431,8 +399,8 @@ func TestGetDocsReviewers(t *testing.T) { c: &Config{ // Docs. DocsReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, + "1": {Owner: true}, + "2": {Owner: true}, }, DocsReviewersOmit: map[string]bool{}, // Admins. @@ -451,8 +419,8 @@ func TestGetDocsReviewers(t *testing.T) { c: &Config{ // Docs. DocsReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, + "1": {Owner: true}, + "2": {Owner: true}, }, DocsReviewersOmit: map[string]bool{ "2": true, @@ -473,8 +441,8 @@ func TestGetDocsReviewers(t *testing.T) { c: &Config{ // Docs. DocsReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, + "1": {Owner: true}, + "2": {Owner: true}, }, DocsReviewersOmit: map[string]bool{}, // Admins. @@ -491,13 +459,12 @@ func TestGetDocsReviewers(t *testing.T) { desc: "preferred code reviewer for docs page", assignments: &Assignments{ c: &Config{ - DocsReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, + "1": {Owner: true}, }, - CodeReviewers: map[string]Reviewer{ - "2": {Team: "Core", Owner: true, PreferredReviewerFor: []string{"docs/pages/server-access"}}, - "3": {Team: "Core", Owner: true}, + CoreReviewers: map[string]Reviewer{ + "2": {Owner: true, PreferredReviewerFor: []string{"docs/pages/server-access"}}, + "3": {Owner: true}, }, }, }, @@ -511,10 +478,9 @@ func TestGetDocsReviewers(t *testing.T) { desc: "preferred code reviewer for docs page with duplicate code reviewers", assignments: &Assignments{ c: &Config{ - - CodeReviewers: map[string]Reviewer{ - "2": {Team: "Core", Owner: true, PreferredReviewerFor: []string{"server-access", "database-access"}}, - "3": {Team: "Core", Owner: true, PreferredReviewerFor: []string{"server-access", "database-access"}}, + CoreReviewers: map[string]Reviewer{ + "2": {Owner: true, PreferredReviewerFor: []string{"server-access", "database-access"}}, + "3": {Owner: true, PreferredReviewerFor: []string{"server-access", "database-access"}}, }, }, }, @@ -529,7 +495,8 @@ func TestGetDocsReviewers(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { e := &env.Environment{ - Author: test.author, + Repository: env.TeleportRepo, + Author: test.author, } reviewers := test.assignments.getDocsReviewers(e, test.files) @@ -543,13 +510,13 @@ func TestCheckExternal(t *testing.T) { r := &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: true}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Core", Owner: false}, - "6": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: true}, + "4": {Owner: false}, + "5": {Owner: false}, + "6": {Owner: false}, }, CodeReviewersOmit: map[string]bool{ "3": true, @@ -637,24 +604,25 @@ func TestCheckInternal(t *testing.T) { r := &Assignments{ c: &Config{ // Code. - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true}, - "2": {Team: "Core", Owner: true}, - "3": {Team: "Core", Owner: true}, - "9": {Team: "Core", Owner: true}, - "4": {Team: "Core", Owner: false}, - "5": {Team: "Core", Owner: false}, - "6": {Team: "Core", Owner: false}, - "8": {Team: "Internal", Owner: false}, - "10": {Team: "Cloud", Owner: false}, - "11": {Team: "Cloud", Owner: false}, - "12": {Team: "Cloud", Owner: false}, - "13": {Team: "Cloud", Owner: true}, - "14": {Team: "Core", Owner: true, PreferredReviewerFor: []string{"docs/pages/server-access"}}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true}, + "2": {Owner: true}, + "3": {Owner: true}, + "9": {Owner: true}, + "4": {Owner: false}, + "5": {Owner: false}, + "6": {Owner: false}, + "14": {Owner: true, PreferredReviewerFor: []string{"docs/pages/server-access"}}, + }, + CloudReviewers: map[string]Reviewer{ + "10": {Owner: false}, + "11": {Owner: false}, + "12": {Owner: false}, + "13": {Owner: true}, }, // Docs. DocsReviewers: map[string]Reviewer{ - "7": {Team: "Core", Owner: true}, + "7": {Owner: true}, }, DocsReviewersOmit: map[string]bool{}, CodeReviewersOmit: map[string]bool{}, @@ -1083,6 +1051,7 @@ func TestCheckInternal(t *testing.T) { Repository: test.repository, Author: test.author, } + changes := env.Changes{ Docs: test.docs, Code: test.code, @@ -1090,9 +1059,11 @@ func TestCheckInternal(t *testing.T) { Release: test.release, ApproverCount: env.DefaultApproverCount, } + if test.singleApproval { changes.ApproverCount = 1 } + err := r.CheckInternal(e, test.reviews, changes, test.files) if test.result { require.NoError(t, err) @@ -1109,13 +1080,11 @@ func TestFromString(t *testing.T) { r, err := FromString(e, reviewers) require.NoError(t, err) - require.EqualValues(t, r.c.CodeReviewers, map[string]Reviewer{ + require.EqualValues(t, r.c.CoreReviewers, map[string]Reviewer{ "1": { - Team: "Core", Owner: true, }, "2": { - Team: "Core", Owner: false, }, }) @@ -1124,11 +1093,9 @@ func TestFromString(t *testing.T) { }) require.EqualValues(t, r.c.DocsReviewers, map[string]Reviewer{ "4": { - Team: "Core", Owner: true, }, "5": { - Team: "Core", Owner: false, }, }) @@ -1151,13 +1118,13 @@ func TestPreferredReviewers(t *testing.T) { assignments := &Assignments{ c: &Config{ Rand: &randStatic{}, - CodeReviewers: map[string]Reviewer{ - "1": {Team: "Core", Owner: true, PreferredReviewerFor: []string{"lib/srv/db", "lib/srv/app"}}, - "2": {Team: "Core", Owner: true, PreferredReviewerFor: []string{"lib/srv/db", "lib/alpn"}}, - "3": {Team: "Core", Owner: true}, - "4": {Team: "Core", Owner: false, PreferredReviewerFor: []string{"lib/srv/app"}}, - "5": {Team: "Core", Owner: false, PreferredReviewerFor: []string{"lib/srv/db"}}, - "6": {Team: "Core", Owner: false}, + CoreReviewers: map[string]Reviewer{ + "1": {Owner: true, PreferredReviewerFor: []string{"lib/srv/db", "lib/srv/app"}}, + "2": {Owner: true, PreferredReviewerFor: []string{"lib/srv/db", "lib/alpn"}}, + "3": {Owner: true}, + "4": {Owner: false, PreferredReviewerFor: []string{"lib/srv/app"}}, + "5": {Owner: false, PreferredReviewerFor: []string{"lib/srv/db"}}, + "6": {Owner: false}, }, }, } @@ -1215,7 +1182,8 @@ func TestPreferredReviewers(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { e := &env.Environment{ - Author: test.author, + Repository: env.TeleportRepo, + Author: test.author, } actual := assignments.getCodeReviewers(e, test.files) sort.Strings(actual)