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

Add support for GitHub Nested Teams 🐣 #8486

Merged
merged 1 commit into from
Jun 30, 2018
Merged

Add support for GitHub Nested Teams 🐣 #8486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
93 changes: 77 additions & 16 deletions prow/cmd/peribolos/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,11 @@ func dumpOrgConfig(client dumpClient, orgName string) (*org.Config, error) {
return nil, fmt.Errorf("failed to list teams: %v", err)
}

out.Teams = make(map[string]org.Team, len(teams))
names := map[int]string{} // what's the name of a team?
idMap := map[int]org.Team{} // metadata for a team
children := map[int][]int{} // what children does it have
var tops []int // what are the top-level teams

for _, t := range teams {
p := org.Privacy(t.Privacy)
d := t.Description
Expand All @@ -230,6 +234,7 @@ func dumpOrgConfig(client dumpClient, orgName string) (*org.Config, error) {
},
Maintainers: []string{},
Members: []string{},
Children: map[string]org.Team{},
}
maintainers, err := client.ListTeamMembers(t.ID, github.RoleMaintainer)
if err != nil {
Expand All @@ -245,8 +250,32 @@ func dumpOrgConfig(client dumpClient, orgName string) (*org.Config, error) {
for _, m := range teamMembers {
nt.Members = append(nt.Members, m.Login)
}
out.Teams[t.Name] = nt

names[t.ID] = t.Name
idMap[t.ID] = nt

if t.Parent == nil { // top level team
tops = append(tops, t.ID)
} else { // add this id to the list of the parent's children
children[t.Parent.ID] = append(children[t.Parent.ID], t.ID)
}
}

var makeChild func(id int) org.Team
makeChild = func(id int) org.Team {
t := idMap[id]
for _, cid := range children[id] {
child := makeChild(cid)
t.Children[names[cid]] = child
}
return t
}

out.Teams = make(map[string]org.Team, len(tops))
Copy link
Member Author

Choose a reason for hiding this comment

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

One last change.. moved this line down here from L222

for _, id := range tops {
out.Teams[names[id]] = makeChild(id)
}

return &out, nil
}

Expand Down Expand Up @@ -360,10 +389,7 @@ func configureOrgMembers(opt options, client orgClient, orgName string, orgConfi
return err
}

if err = configureMembers(have, want, adder, remover); err != nil {
return err
}
return nil
return configureMembers(have, want, adder, remover)
}

type memberships struct {
Expand Down Expand Up @@ -633,20 +659,39 @@ func configureOrg(opt options, client *github.Client, orgName string, orgConfig
}

for name, team := range orgConfig.Teams {
gt, ok := githubTeams[name]
if !ok { // configureTeams is buggy if this is the case
return fmt.Errorf("%s not found in id list", name)
err := configureTeamAndMembers(client, githubTeams, name, orgName, team, nil)
if err != nil {
return fmt.Errorf("failed to configure %s teams: %v", orgName, err)
}
}
return nil
}

func configureTeamAndMembers(client *github.Client, githubTeams map[string]github.Team, name, orgName string, team org.Team, parent *int) error {
gt, ok := githubTeams[name]
if !ok { // configureTeams is buggy if this is the case
return fmt.Errorf("%s not found in id list", name)
}

// Configure team metadata
err := configureTeam(client, orgName, name, team, gt)
// Configure team metadata
err := configureTeam(client, orgName, name, team, gt, parent)
if err != nil {
return fmt.Errorf("failed to update %s metadata: %v", name, err)
}

// Configure team members
err = configureTeamMembers(client, gt.ID, team)
if err != nil {
return fmt.Errorf("failed to update %s members: %v", name, err)
}

for childName, childTeam := range team.Children {
err = configureTeamAndMembers(client, githubTeams, childName, orgName, childTeam, &gt.ID)
if err != nil {
return fmt.Errorf("failed to update %s: %v", orgName, err)
return fmt.Errorf("failed to update %s child teams: %v", name, err)
}

// Add/remove/update members to the team.
return configureTeamMembers(client, gt.ID, team)
}

return nil
}

Expand All @@ -655,7 +700,7 @@ type editTeamClient interface {
}

// configureTeam patches the team name/description/privacy when values differ
func configureTeam(client editTeamClient, orgName, teamName string, team org.Team, gt github.Team) error {
func configureTeam(client editTeamClient, orgName, teamName string, team org.Team, gt github.Team, parent *int) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand the motivation behind / value of changing this to parent *int instead of int.

Copy link
Member Author

Choose a reason for hiding this comment

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

0 is technically a valid parent team ID. We need to be able to tell the difference between 0 and nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks! Can you include a unit test for removing the parent from a team then?

// Do we need to reconfigure any team settings?
patch := false
if gt.Name != teamName {
Expand All @@ -675,6 +720,22 @@ func configureTeam(client editTeamClient, orgName, teamName string, team org.Tea
} else {
gt.Privacy = ""
}
// doesn't have parent in github, but has parent in config
if gt.Parent == nil && parent != nil {
patch = true
gt.ParentTeamID = parent
}
if gt.Parent != nil { // has parent in github ...
if parent == nil { // ... but doesn't need one
patch = true
gt.Parent = nil
gt.ParentTeamID = parent
} else if gt.Parent.ID != *parent { // but it's different than the config
patch = true
gt.Parent = nil
gt.ParentTeamID = parent
}
}

if patch { // yes we need to patch
if _, err := client.EditTeam(gt); err != nil {
Expand Down
87 changes: 83 additions & 4 deletions prow/cmd/peribolos/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,13 @@ func (c *fakeTeamClient) EditTeam(team github.Team) (*github.Team, error) {
if team.Privacy != "" {
t.Privacy = team.Privacy
}
if team.ParentTeamID != nil {
t.Parent = &github.Team{
ID: *team.ParentTeamID,
}
} else {
t.Parent = nil
}
c.teams[id] = t
return &t, nil
}
Expand Down Expand Up @@ -974,10 +981,12 @@ func TestConfigureTeam(t *testing.T) {
pfail := org.Privacy(fail)
whatev := "whatever"
secret := org.Secret
parent := 2
cases := []struct {
name string
err bool
teamName string
parent *int
Copy link
Contributor

Choose a reason for hiding this comment

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

where do you use this value?

config org.Team
github github.Team
expected github.Team
Expand All @@ -1000,6 +1009,7 @@ func TestConfigureTeam(t *testing.T) {
{
name: "patch team when description changes",
teamName: whatev,
parent: nil,
config: org.Team{
TeamMetadata: org.TeamMetadata{
Description: &cur,
Expand All @@ -1019,6 +1029,7 @@ func TestConfigureTeam(t *testing.T) {
{
name: "patch team when privacy changes",
teamName: whatev,
parent: nil,
config: org.Team{
TeamMetadata: org.TeamMetadata{
Privacy: &secret,
Expand All @@ -1035,9 +1046,48 @@ func TestConfigureTeam(t *testing.T) {
Privacy: string(secret),
},
},
{
name: "patch team when parent changes",
teamName: whatev,
parent: &parent,
config: org.Team{},
github: github.Team{
ID: 3,
Name: whatev,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure the parent ID is the only bad value.

By making config.Name != github.Name also differ it is no longer clear that ParentTeamID change causes an update (might just be luck due to the name diff).

Parent: &github.Team{
ID: 4,
},
},
expected: github.Team{
ID: 3,
Name: whatev,
Parent: &github.Team{
ID: 2,
},
},
},
{
name: "patch team when parent removed",
teamName: whatev,
parent: nil,
config: org.Team{},
github: github.Team{
ID: 3,
Name: whatev,
Parent: &github.Team{
ID: 2,
},
},
expected: github.Team{
ID: 3,
Name: whatev,
Parent: nil,
},
},
{
name: "do not patch team when values are the same",
teamName: fail,
parent: &parent,
config: org.Team{
TeamMetadata: org.TeamMetadata{
Description: &fail,
Expand All @@ -1049,17 +1099,24 @@ func TestConfigureTeam(t *testing.T) {
Name: fail,
Description: fail,
Privacy: fail,
Parent: &github.Team{
ID: 2,
},
},
expected: github.Team{
ID: 4,
Name: fail,
Description: fail,
Privacy: fail,
Parent: &github.Team{
ID: 2,
},
},
},
{
name: "fail to patch team",
teamName: "team",
parent: nil,
config: org.Team{
TeamMetadata: org.TeamMetadata{
Description: &fail,
Expand All @@ -1077,7 +1134,7 @@ func TestConfigureTeam(t *testing.T) {
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
fc := makeFakeTeamClient(tc.github)
err := configureTeam(fc, fakeOrg, tc.teamName, tc.config, tc.github)
err := configureTeam(fc, fakeOrg, tc.teamName, tc.config, tc.github, tc.parent)
switch {
case err != nil:
if !tc.err {
Expand All @@ -1086,7 +1143,7 @@ func TestConfigureTeam(t *testing.T) {
case tc.err:
t.Errorf("failed to receive expected error")
case !reflect.DeepEqual(fc.teams[tc.expected.ID], tc.expected):
t.Errorf("actual %v != expected %v", fc.teams[tc.expected.ID], tc.expected)
t.Errorf("actual %+v != expected %+v", fc.teams[tc.expected.ID], tc.expected)
}
})
}
Expand Down Expand Up @@ -1567,7 +1624,7 @@ func TestDumpOrgConfig(t *testing.T) {
MembersCanCreateRepositories: yes,
DefaultRepositoryPermission: string(perm),
},
members: []string{"george", "jungle"},
members: []string{"george", "jungle", "banana"},
admins: []string{"james", "giant", "peach"},
teams: []github.Team{
{
Expand All @@ -1579,14 +1636,24 @@ func TestDumpOrgConfig(t *testing.T) {
ID: 6,
Name: "enemies",
},
{
ID: 7,
Name: "archenemies",
Parent: &github.Team{
ID: 6,
Name: "enemies",
},
},
},
teamMembers: map[int][]string{
5: {"george", "james"},
6: {"george"},
7: {},
},
maintainers: map[int][]string{
5: {},
6: {"giant", "jungle"},
7: {"banana"},
},
expected: org.Config{
Metadata: org.Metadata{
Expand All @@ -1609,6 +1676,7 @@ func TestDumpOrgConfig(t *testing.T) {
},
Members: []string{"george", "james"},
Maintainers: []string{},
Children: map[string]org.Team{},
},
"enemies": {
TeamMetadata: org.TeamMetadata{
Expand All @@ -1617,9 +1685,20 @@ func TestDumpOrgConfig(t *testing.T) {
},
Members: []string{"george"},
Maintainers: []string{"giant", "jungle"},
Children: map[string]org.Team{
"archenemies": {
TeamMetadata: org.TeamMetadata{
Description: &empty,
Privacy: &pub,
},
Members: []string{},
Maintainers: []string{"banana"},
Children: map[string]org.Team{},
},
},
},
},
Members: []string{"george", "jungle"},
Members: []string{"george", "jungle", "banana"},
Admins: []string{"james", "giant", "peach"},
},
},
Expand Down
5 changes: 3 additions & 2 deletions prow/config/org/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ type TeamMetadata struct {
// Team declares metadata as well as its poeple.
type Team struct {
TeamMetadata
Members []string `json:"members,omitempty"`
Maintainers []string `json:"maintainers,omitempty"`
Members []string `json:"members,omitempty"`
Maintainers []string `json:"maintainers,omitempty"`
Children map[string]Team `json:"teams,omitempty"`

Previously []string `json:"previously,omitempty"`
}
Expand Down
18 changes: 13 additions & 5 deletions prow/github/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1591,8 +1591,11 @@ func (c *Client) CreateTeam(org string, team Team) (*Team, error) {
path := fmt.Sprintf("/orgs/%s/teams", org)
var retTeam Team
_, err := c.request(&request{
method: http.MethodPost,
path: path,
method: http.MethodPost,
path: path,
// This accept header enables the nested teams preview.
// https://developer.github.com/changes/2017-08-30-preview-nested-teams/
accept: "application/vnd.github.hellcat-preview+json",
requestBody: &team,
exitCodes: []int{201},
}, &retTeam)
Expand All @@ -1612,8 +1615,11 @@ func (c *Client) EditTeam(team Team) (*Team, error) {
var retTeam Team
path := fmt.Sprintf("/teams/%d", id)
_, err := c.request(&request{
method: http.MethodPatch,
path: path,
method: http.MethodPatch,
path: path,
// This accept header enables the nested teams preview.
// https://developer.github.com/changes/2017-08-30-preview-nested-teams/
accept: "application/vnd.github.hellcat-preview+json",
requestBody: &team,
exitCodes: []int{201},
}, &retTeam)
Expand Down Expand Up @@ -1646,7 +1652,9 @@ func (c *Client) ListTeams(org string) ([]Team, error) {
var teams []Team
err := c.readPaginatedResults(
path,
"",
// This accept header enables the nested teams preview.
// https://developer.github.com/changes/2017-08-30-preview-nested-teams/
"application/vnd.github.hellcat-preview+json",
func() interface{} {
return &[]Team{}
},
Expand Down
Loading