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.

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

Feature: Team mentor #28

Merged
merged 1 commit into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cmd/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion cmd/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
6 changes: 4 additions & 2 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,17 @@ import (
)

var (
orgName string
configFilename string
orgName string
configFilename string
overrideFilename string
)

func init() {
flag := rootCmd.PersistentFlags()

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{
Expand Down
2 changes: 1 addition & 1 deletion cmd/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/reviews.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
41 changes: 39 additions & 2 deletions cmd/teams.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
func init() {
rootCmd.AddCommand(addTeamsCmd)
rootCmd.AddCommand(setTeamsUsersCmd)
rootCmd.AddCommand(setTeamsMentorsCmd)
}

var addTeamsCmd = &cobra.Command{
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion cmd/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
55 changes: 55 additions & 0 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package config

import (
"fmt"
"os"
"strings"

"github.com/cilium/team-manager/pkg/slices"
Expand Down Expand Up @@ -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) {
Expand All @@ -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.
Expand Down Expand Up @@ -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"`

Expand Down
5 changes: 5 additions & 0 deletions pkg/config/sanitycheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 12 additions & 0 deletions pkg/config/sort.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <
Expand Down
38 changes: 37 additions & 1 deletion pkg/persistence/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading