diff --git a/README.md b/README.md index 0913755..9c265c0 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,9 @@ teams: - aanm - borkmann - joestringer + # Optional list of team mentors who will not be auto-assigned PRs for review + mentors: + - aanm codeReviewAssignment: # algorithm, currently can be LOAD_BALANCE or ROUND_ROBIN. algorithm: LOAD_BALANCE diff --git a/cmd/init.go b/cmd/init.go index e956b2d..a6c8c5f 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -39,7 +39,7 @@ var initCmd = &cobra.Command{ return fmt.Errorf("unable to initialize manager %w", err) } - if _, err := persistence.LoadState(configFilename); err == nil { + if _, err := persistence.LoadState(configFilename, overrideFilename); err == nil { fmt.Printf("Configuration file %q already exists\n", configFilename) return nil } else if !errors.Is(err, os.ErrNotExist) { diff --git a/cmd/lint.go b/cmd/lint.go index 4e00af6..27214c2 100644 --- a/cmd/lint.go +++ b/cmd/lint.go @@ -22,7 +22,7 @@ var checkCmd = &cobra.Command{ Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, _ []string) error { - localCfg, err := persistence.LoadState(configFilename) + localCfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } diff --git a/cmd/main.go b/cmd/main.go index b9d51d3..7a1d736 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -23,8 +23,9 @@ import ( ) var ( - orgName string - configFilename string + orgName string + configFilename string + overrideFilename string ) func init() { @@ -32,6 +33,7 @@ func init() { flag.StringVar(&orgName, "org", "cilium", "GitHub organization name") flag.StringVar(&configFilename, "config-filename", "team-assignments.yaml", "Config filename") + flag.StringVar(&overrideFilename, "override-filename", "", "Team Override filename") } var rootCmd = &cobra.Command{ diff --git a/cmd/push.go b/cmd/push.go index 3b0162f..4e7ddcf 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -37,7 +37,7 @@ var pushCmd = &cobra.Command{ Short: "Update team assignments in GitHub from local files", Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, _ []string) error { - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } diff --git a/cmd/reviews.go b/cmd/reviews.go index 6bfb1cb..e6773a8 100644 --- a/cmd/reviews.go +++ b/cmd/reviews.go @@ -23,7 +23,7 @@ var addPTOCmd = &cobra.Command{ Short: "Exclude user from code review assignments", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } @@ -44,7 +44,7 @@ var removePTOCmd = &cobra.Command{ Short: "Include user in code review assignments", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } diff --git a/cmd/status.go b/cmd/status.go index 3ea9287..a6e2c15 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -25,7 +25,7 @@ var statusCmd = &cobra.Command{ Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, _ []string) error { - localCfg, err := persistence.LoadState(configFilename) + localCfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } diff --git a/cmd/sync.go b/cmd/sync.go index 258f40b..27551f4 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -23,7 +23,7 @@ var syncCmd = &cobra.Command{ Short: "Merges the configuration from GitHub into the local configuration", Args: cobra.ExactArgs(0), RunE: func(cmd *cobra.Command, _ []string) error { - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } diff --git a/cmd/teams.go b/cmd/teams.go index 1ba047b..f2b4872 100644 --- a/cmd/teams.go +++ b/cmd/teams.go @@ -19,6 +19,7 @@ import ( func init() { rootCmd.AddCommand(addTeamsCmd) rootCmd.AddCommand(setTeamsUsersCmd) + rootCmd.AddCommand(setTeamsMentorsCmd) } var addTeamsCmd = &cobra.Command{ @@ -31,7 +32,7 @@ var addTeamsCmd = &cobra.Command{ return fmt.Errorf("failed to create github client: %w", err) } - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } @@ -52,7 +53,7 @@ var setTeamsUsersCmd = &cobra.Command{ Short: "Set members of a team in local configuration", Args: cobra.MinimumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } @@ -68,6 +69,27 @@ var setTeamsUsersCmd = &cobra.Command{ return nil }, } +var setTeamsMentorsCmd = &cobra.Command{ + Use: "set-team-mentors TEAM [USER ...]", + Short: "Set mentors of a team in local configuration", + Args: cobra.MinimumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cfg, err := persistence.LoadState(configFilename, overrideFilename) + if err != nil { + return fmt.Errorf("failed to load local state: %w", err) + } + + if err = setTeamMentors(args[0], args[1:], cfg); err != nil { + return fmt.Errorf("failed to set team mentors: %w", err) + } + + if err = persistence.StoreState(configFilename, cfg); err != nil { + return fmt.Errorf("failed to store state to config: %w", err) + } + + return nil + }, +} func addTeamsToConfig(ctx context.Context, addTeams []string, cfg *config.Config, ghClient *gh.Client) error { for _, addTeam := range addTeams { @@ -129,6 +151,21 @@ func setTeamMembers(team string, users []string, cfg *config.Config) error { return nil } +func setTeamMentors(team string, users []string, cfg *config.Config) error { + mentors, err := findUsers(cfg, users) + if err != nil { + return fmt.Errorf("unable to find users: %w", err) + } + teamConfig, ok := cfg.AllTeams[team] + if !ok { + return fmt.Errorf("unknown team %q", team) + } + teamConfig.Mentors = stringset.New(mentors...).Elements() + cfg.AllTeams[team] = teamConfig + + return nil +} + func addTeamMembers(team string, users []string, cfg *config.Config) error { teamConfig, ok := cfg.AllTeams[team] if !ok { diff --git a/cmd/users.go b/cmd/users.go index 2d6c202..6f49a92 100644 --- a/cmd/users.go +++ b/cmd/users.go @@ -36,7 +36,7 @@ var addUsersCmd = &cobra.Command{ return fmt.Errorf("failed to create github client: %w", err) } - cfg, err := persistence.LoadState(configFilename) + cfg, err := persistence.LoadState(configFilename, overrideFilename) if err != nil { return fmt.Errorf("failed to load local state: %w", err) } diff --git a/pkg/config/config.go b/pkg/config/config.go index c644b67..c9007a6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -16,6 +16,7 @@ package config import ( "fmt" + "os" "strings" "github.com/cilium/team-manager/pkg/slices" @@ -90,12 +91,44 @@ type Config struct { // maps the team name to its config. GitHub doesn't allow duplicated team // names, so we can do safely do this. AllTeams map[string]*TeamConfig `json:"-" yaml:"-"` + + // OverrideTeams is an index of teams in the override file + // maps the team name to its config. GitHub doesn't allow duplicated team + // names, so we can do safely do this. + TeamOverrides map[string]*OverrideTeamConfig `json:"-" yaml:"-"` +} + +// This will hold the information from the Team Override File +type OverrideConfig struct { + // Teams maps the github team name to a OverrideTeamConfig. + Teams map[string]*OverrideTeamConfig `json:"teams,omitempty" yaml:"teams,omitempty"` +} + +// OverrideTeamConfig is intended to be made public containing only github usernames and team names +type OverrideTeamConfig struct { + // Members is a list of users that belong to this team. + Members []string `json:"members,omitempty" yaml:"members,omitempty"` + // Mentors is a list of users that belong to this team that will be excluded from auto review assignments. + Mentors []string `json:"mentors,omitempty" yaml:"mentors,omitempty"` } func (c *Config) IndexTeams() { allTeams := map[string]*TeamConfig{} getAllTeams(c.Teams, allTeams) + applyTeamOverrides(c.TeamOverrides, allTeams) c.AllTeams = allTeams + +} + +func applyTeamOverrides(teams map[string]*OverrideTeamConfig, allTeams map[string]*TeamConfig) { + for teamName, team := range teams { + if _, ok := allTeams[teamName]; ok { + allTeams[teamName].Members = team.Members + allTeams[teamName].Mentors = team.Mentors + } else { + fmt.Fprintf(os.Stderr, "Override Warning: team %s missing from config\n", teamName) + } + } } func getAllTeams(teams, allTeams map[string]*TeamConfig) { @@ -118,6 +151,24 @@ func (c *Config) Merge(other *Config) (*Config, error) { slices.Remove(other.ExcludeCRAFromAllTeams, login) } } + // Keep mentors since we can't fetch this information + // from GitHub. + for otherTeamName, otherTeam := range other.AllTeams { + team, ok := c.AllTeams[otherTeamName] + if !ok { + continue + } + otherTeam.Mentors = nil + for _, mentor := range team.Mentors { + for _, member := range otherTeam.Members { + if member == mentor { + otherTeam.Mentors = append( + otherTeam.Mentors, mentor) + break + } + } + } + } // Keep the code review assignment since we can't fetch this information // from GitHub. @@ -212,6 +263,10 @@ type TeamConfig struct { // Members is a list of users that belong to this team. Members []string `json:"members,omitempty" yaml:"members,omitempty"` + // Mentors is a list of users that belong to this team, but opt out of code review notifications + // Note 1: Mentors _must_ be in the member list. Lint will warn if they are not + Mentors []string `json:"mentors,omitempty" yaml:"mentors,omitempty"` + // CodeReviewAssignment is the code review assignment configuration of this team CodeReviewAssignment CodeReviewAssignment `json:"codeReviewAssignment,omitempty" yaml:"codeReviewAssignment,omitempty"` diff --git a/pkg/config/sanitycheck.go b/pkg/config/sanitycheck.go index 3799aea..6055273 100644 --- a/pkg/config/sanitycheck.go +++ b/pkg/config/sanitycheck.go @@ -30,6 +30,11 @@ func SanityCheck(cfg *Config) error { return fmt.Errorf("member %q from team %q does not belong to organization", member, teamName) } } + for _, mentor := range team.Mentors { + if _, ok := cfg.Members[mentor]; !ok { + return fmt.Errorf("mentor %q from team %q does not belong to organization", mentor, teamName) + } + } for _, xMember := range team.CodeReviewAssignment.ExcludedMembers { if _, ok := cfg.Members[xMember.Login]; !ok { return fmt.Errorf("member %q from code review assignment of team %q does not belong to organization", xMember.Login, teamName) diff --git a/pkg/config/sort.go b/pkg/config/sort.go index 1418252..c78db28 100644 --- a/pkg/config/sort.go +++ b/pkg/config/sort.go @@ -40,6 +40,18 @@ func SortConfig(cfg *Config) { } sort.Strings(team.Members) + // Remove and sort and duplicated team mentors + teamMentors := make(map[string]struct{}, len(team.Mentors)) + for _, teamMentor := range team.Mentors { + teamMentors[teamMentor] = struct{}{} + } + + team.Mentors = make([]string, 0, len(teamMentors)) + for teamMentor := range teamMentors { + team.Mentors = append(team.Mentors, teamMentor) + } + sort.Strings(team.Mentors) + // sort excluded members as well sort.Slice(team.CodeReviewAssignment.ExcludedMembers, func(i, j int) bool { return team.CodeReviewAssignment.ExcludedMembers[i].Login < diff --git a/pkg/persistence/storage.go b/pkg/persistence/storage.go index 53edd5c..3830ca3 100644 --- a/pkg/persistence/storage.go +++ b/pkg/persistence/storage.go @@ -38,7 +38,43 @@ func StoreState(file string, cfg *config.Config) error { return renameio.WriteFile(file, data, 0o666) } -func LoadState(file string) (*config.Config, error) { +func LoadState(file, overrides string) (*config.Config, error) { + f, err := os.OpenFile(file, os.O_RDONLY, 0440) + if err != nil { + return nil, err + } + defer f.Close() + + storedConfig := config.Config{} + err = yaml.NewDecoder(f).Decode(&storedConfig) + if err != nil { + return nil, err + } + + if len(overrides) > 0 { + o, err := os.OpenFile(overrides, os.O_RDONLY, 0440) + if err != nil { + return nil, err + } + defer o.Close() + storedOverrides := config.OverrideConfig{} + err = yaml.NewDecoder(o).Decode(&storedOverrides) + if err != nil { + return nil, err + } + storedConfig.TeamOverrides = storedOverrides.Teams + } + + // Index the teams into AllTeams for easy access. + storedConfig.IndexTeams() + + // Set the parent names for all teams. + config.SetParentNames(storedConfig.AllTeams) + + return &storedConfig, nil +} + +func LoadOverrides(file string) (*config.Config, error) { f, err := os.OpenFile(file, os.O_RDONLY, 0440) if err != nil { return nil, err diff --git a/pkg/team/manager.go b/pkg/team/manager.go index 80e91fd..278e203 100644 --- a/pkg/team/manager.go +++ b/pkg/team/manager.go @@ -486,8 +486,12 @@ func (tm *Manager) CheckUserStatus(ctx context.Context, localCfg *config.Config) } for teamName, team := range localCfg.AllTeams { - excludedTeamMembers := map[string]struct{}{} + excludedTeamMentors := map[string]struct{}{} + for _, xMentor := range team.Mentors { + excludedTeamMentors[xMentor] = struct{}{} + } + excludedTeamMembers := map[string]struct{}{} for _, xMember := range team.CodeReviewAssignment.ExcludedMembers { excludedTeamMembers[xMember.Login] = struct{}{} } @@ -509,6 +513,11 @@ func (tm *Manager) CheckUserStatus(ctx context.Context, localCfg *config.Config) unavailableMembers++ continue } + _, isExcluded = excludedTeamMentors[member] + if isExcluded { + unavailableMembers++ + continue + } } fmt.Printf("Team %q has the following active member ratio: %d/%d.\n", teamName, len(team.Members)-unavailableMembers, len(team.Members)) @@ -523,19 +532,37 @@ func (tm *Manager) CheckUserStatus(ctx context.Context, localCfg *config.Config) } fmt.Printf("Team %q with %d members doesn't have enough reviewers:\n", teamName, len(team.Members)) for _, member := range team.Members { + statusString := "" _, isBusy := busyMembers[member] if isBusy { - fmt.Printf(" - %s - busy\n", member) - continue + if len(statusString) > 0 { + statusString = statusString + ", " + } + statusString = statusString + "busy" } _, isExcluded := excludedMembers[member] if isExcluded { - fmt.Printf(" - %s - excluded\n", member) - continue + if len(statusString) > 0 { + statusString = statusString + ", " + } + statusString = statusString + "org_excluded" } _, isExcluded = excludedTeamMembers[member] if isExcluded { - fmt.Printf(" - %s - excluded\n", member) + if len(statusString) > 0 { + statusString = statusString + ", " + } + statusString = statusString + "team_excluded" + } + _, isExcluded = excludedTeamMentors[member] + if isExcluded { + if len(statusString) > 0 { + statusString = statusString + ", " + } + statusString = statusString + "team_mentor" + } + if len(statusString) > 0 { + fmt.Printf(" - %s - %s\n", member, statusString) continue } fmt.Printf(" - %s - ok\n", member) diff --git a/pkg/team/push.go b/pkg/team/push.go index bbb6c4e..7a9f978 100644 --- a/pkg/team/push.go +++ b/pkg/team/push.go @@ -501,7 +501,7 @@ func (tm *Manager) pushCodeReviewAssignments(ctx context.Context, localCfg *conf for _, teamName := range teamNames { storedTeam := localCfg.AllTeams[teamName] cra := storedTeam.CodeReviewAssignment - usersIDs := getExcludedUsers(teamName, localCfg.Members, cra.ExcludedMembers, localCfg.ExcludeCRAFromAllTeams) + usersIDs := getExcludedUsers(teamName, localCfg.Members, storedTeam.Mentors, cra.ExcludedMembers, localCfg.ExcludeCRAFromAllTeams) input := github.UpdateTeamReviewAssignmentInput{ Algorithm: cra.Algorithm, @@ -624,6 +624,7 @@ func (tm *Manager) pushMembers(ctx context.Context, force, dryRun bool, localCfg for _, team := range localCfg.AllTeams { team.Members = slices.Remove(team.Members, member) + team.Mentors = slices.Remove(team.Mentors, member) team.CodeReviewAssignment.ExcludedMembers = config.RemoveExcludedMember(team.CodeReviewAssignment.ExcludedMembers, member) } diff --git a/pkg/team/utils.go b/pkg/team/utils.go index e9b645c..b35fb42 100644 --- a/pkg/team/utils.go +++ b/pkg/team/utils.go @@ -26,8 +26,16 @@ import ( // getExcludedUsers returns a list of all users that should be excluded for the // given team. -func getExcludedUsers(teamName string, members map[string]config.User, excTeamMembers []config.ExcludedMember, excAllTeams []string) []githubv4.ID { - m := make(map[githubv4.ID]struct{}, len(excTeamMembers)+len(excAllTeams)) +func getExcludedUsers(teamName string, members map[string]config.User, mentors []string, excTeamMembers []config.ExcludedMember, excAllTeams []string) []githubv4.ID { + m := make(map[githubv4.ID]struct{}, len(members)+len(excTeamMembers)+len(excAllTeams)) + for _, member := range mentors { + user, ok := members[member] + if !ok { + fmt.Printf("[ERROR] mentor %q from team %s, not found in the list of team members in the organization\n", member, teamName) + continue + } + m[user.ID] = struct{}{} + } for _, member := range excTeamMembers { user, ok := members[member.Login] if !ok {