Add support for GitHub Nested Teams 🐣 #8486

cblecker · 2018-06-27T21:52:03Z

Add support for Github Nested Teams to peribolos.
https://blog.github.com/2017-06-13-nested-teams-add-depth-to-your-team-structure/

example config:

teams:
  sig-foo:
    description: foo enthusiasts
    members:
    - george
    - jane
    privacy: closed
    teams:
      sig-foo-leads:
      members:
      - george
      sig-foo-pr-reviews:
      members:
      - jane

fejta · 2018-06-27T21:58:28Z

prow/config/org/org.go

-	Privacy     *Privacy `json:"privacy,omitempty"`
+	Description  *string  `json:"description,omitempty"`
+	Privacy      *Privacy `json:"privacy,omitempty"`
+	ParentTeamID *int     `json:"parent_team_id,omitempty"`


Remove this and add a Children map[string]Team field in Team (or maybe Teams)

We would want the config to be something like:

teams: sig-foo: description: foo enthusiasts teams: sig-foo-leads: members: - george sig-foo-pr-reviews: members: - jane

fejta · 2018-06-27T21:58:56Z

prow/github/types.go

@@ -579,6 +579,16 @@ type Team struct {
 	Name        string `json:"name,omitempty"`
 	Description string `json:"description,omitempty"`
 	Privacy     string `json:"privacy,omitempty"`
+	Parent      *Team  `json:"parent,omitempty"`


ParentTeamID here, get rid of the unnecessary NewTeam struct

fejta · 2018-06-27T22:09:49Z

prow/cmd/peribolos/main.go

@@ -651,7 +657,7 @@ func configureOrg(opt options, client *github.Client, orgName string, orgConfig
 }

 type editTeamClient interface {
-	EditTeam(team github.Team) (*github.Team, error)
+	EditTeam(team github.NewTeam) (*github.Team, error)
 }

 // configureTeam patches the team name/description/privacy when values differ


I would change configureTeam() to receive the parent int and do something like:

func configureTeam(client editTeamClient, orgName, teamName string, team org.Team, gt github.Team, parent int) error { // ... if parent > 0 && gt.ParentID != parent { gt.ParentID = parent patch = true } // ... }

Then in configureOrg() the for name, team := range orgConfig.Teams { ... } probably needs to be in its own recursive function:

func configureOrg(...) { // ... for name, team := range orgConfig.Teams { // move what's currently here into the configureTeamAndMembers() configureTeamAndMembers(client, githubTeams, name, team, 0) // top level teams have no parent } ... } func configureTeamAndMembers(client, githubTeams map[string]github.Team, name string, team org.Team, parent int) error { gt, ok := githubTeams[name] err := configureTeam(client, orgName, name, team, parent) err := configureTeamMembers(client, gt.ID, team) for childName, childTeam := range team.Teams { err := configureTeamAndMembers(client, githubTeams, childName, childTeam, gt.ID) } }

fejta · 2018-06-27T22:12:22Z

prow/cmd/peribolos/main.go

@@ -231,6 +231,9 @@ func dumpOrgConfig(client dumpClient, orgName string) (*org.Config, error) {
 			Maintainers: []string{},
 			Members:     []string{},
 		}
+		if t.Parent.ID != 0 {


This should construct the recursive tree.

fejta

Here's a skeleton for how we can convert the github response into the tree we want for --dump

fejta · 2018-06-28T22:05:47Z

prow/cmd/peribolos/main.go

-		}
-		for _, m := range teamMembers {
-			nt.Members = append(nt.Members, m.Login)
+	out.Teams = make(map[string]org.Team)


I think the general goal here is:

a) Convert github.Team metadata into org.Team metadata
b) Figure out what children a given parent has
c) Create the tree of children, starting at ancestors.

So then we wind up with something like this:

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 // first create all the org.Team items for _, t := range teams { nt := org.Team{ TeamMetadata: org.TeamMetadata{ Description: t.Description }, Maintainer: client.ListTeamMembers(t.ID) } names[t.ID] = t.Name idMap[t.ID] = nt if t.ParentID == 0 { // top level team tops = append(tops, t.ID) } else { // add this id to the list of the parent's children children[t.ParentID] = append(children[t.ParentID], t.ID) } } makeChild := func(id int) org.Team { t := idMap[id] for _, cid := range children[id] { child := makeChild(cid) t.Children = append(t.Children, child) } return t } for _, id := range tops { out.Teams[names[id]] = makeChild(id) }

cblecker · 2018-06-29T21:46:02Z

@fejta I think this is ready for another review. Got the dump working (thanks for the help!). Example from kubernetes-sigs:

  test-parent-team:
    description: ""
    maintainers:
    - cblecker
    privacy: closed
    teams:
      test-child-team:
        description: ""
        maintainers:
        - cblecker
        privacy: closed

fejta

🍾 very cool

A couple comments

fejta · 2018-06-29T22:33:39Z

prow/github/types.go

+	Name         string `json:"name,omitempty"`
+	Description  string `json:"description,omitempty"`
+	Privacy      string `json:"privacy,omitempty"`
+	Parent       *Team  `json:"parent,omitempty"`


Maybe note with a comment that this is only relevant for responses

fejta · 2018-06-29T22:37:01Z

prow/cmd/peribolos/main_test.go

@@ -1077,7 +1077,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, 0)


Will you s/0/tc.parentID and ensure cover this in "do not patch team when values are the same" and have a "patch team when parent changes"

fejta · 2018-06-29T22:37:30Z

prow/cmd/peribolos/main.go

@@ -675,6 +721,11 @@ func configureTeam(client editTeamClient, orgName, teamName string, team org.Tea
 	} else {
 		gt.Privacy = ""
 	}
+	if parent > 0 && gt.ParentTeamID != &parent {


Can I make a child team into a top-level team by later setting its parentID to 0? I think so, in which case we probably don't need the parent > 0 check?

no.. you actually need to send parent_team_id: null for that to work. removed the omitempty so it will handle this case.

cblecker · 2018-06-30T00:48:48Z

@fejta Addressed comments, but having some issues with the tests.. would you mind having a look at 9a6de59 ? there might be something dumb and obvious I'm missing.. both the test cases you call out are failing:

--- FAIL: TestConfigureTeam (0.00s)
    --- FAIL: TestConfigureTeam/patch_team_when_parent_changes (0.00s)
        main_test.go:1130: actual {3 whatever   0xc420129680 <nil>} != expected {3 whatever   <nil> 0xc42023b0b0}
    --- FAIL: TestConfigureTeam/do_not_patch_team_when_values_are_the_same (0.00s)
        main_test.go:1125: unexpected error: failed to edit random-org team 4(fail): injected name failure

fejta · 2018-06-30T01:55:40Z

prow/cmd/peribolos/main.go

@@ -655,7 +701,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 {


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

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

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

fejta · 2018-06-30T01:56:21Z

prow/cmd/peribolos/main_test.go

@@ -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 {
+		team.Parent = &github.Team{


Wrong lvalue

I'm confused.. I don't think it is. The goal here in the fake client is to take ParentTeamID, and set the Parent struct to contain a fake team with that matching ID.

EDIT: I get it now.

fejta · 2018-06-30T01:56:44Z

prow/cmd/peribolos/main_test.go

 	cases := []struct {
 		name     string
 		err      bool
 		teamName string
+		parent   *int


where do you use this value?

fejta · 2018-06-30T01:58:16Z

prow/cmd/peribolos/main_test.go

+			config:   org.Team{},
+			github: github.Team{
+				ID:   3,
+				Name: whatev,


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).

fejta · 2018-06-30T01:58:46Z

prow/cmd/peribolos/main_test.go

+				Name: whatev,
+				Parent: &github.Team{
+					ID:   4,
+					Name: "sup",


Technically Name is unnecessary here. Consider removing for clarity

cblecker · 2018-06-30T03:29:45Z

@fejta oooookay. I think I got this figured out, and the tests working. PTAL, and I'll squash after.. thanks again for the pointers! :)

fejta · 2018-06-30T04:14:57Z

commit includes an emoji, auto
/approve

fejta

/lgtm
/hold

cblecker · 2018-06-30T05:56:16Z

Squashified!

BenTheElder · 2018-06-30T18:28:03Z

/test pull-test-infra-bazel-canary

BenTheElder · 2018-06-30T18:40:01Z

Sorry for the noise, this is the failure I'm looking for, after #8525 hopefully we'll have the fix confirmed.

…

On Sat, Jun 30, 2018 at 11:34 AM k8s-ci-robot ***@***.***> wrote: @cblecker <https://github.com/cblecker>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-test-infra-bazel-canary 7f86b7a <7f86b7a> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/test-infra/8486/pull-test-infra-bazel-canary/197/> /test pull-test-infra-bazel-canary Full PR test history <https://k8s-gubernator.appspot.com/pr/test-infra/8486>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/cblecker>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/test-infra/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4Bqx3OSeCW6Qy4QSqfM3e1DH0Uh3_Yks5uB8SlgaJpZM4U6cQB> .

cblecker · 2018-06-30T18:46:21Z

prow/cmd/peribolos/main.go

+		return t
+	}
+
+	out.Teams = make(map[string]org.Team, len(tops))


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

BenTheElder · 2018-06-30T19:13:27Z

/test pull-test-infra-bazel-canary

fejta

/lgtm

k8s-ci-robot · 2018-06-30T20:40:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, fejta

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [fejta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cblecker · 2018-06-30T23:45:58Z

/hold cancel

fejta · 2018-06-30T23:50:32Z

/retest

cblecker added the area/prow Issues or PRs related to prow label Jun 27, 2018

cblecker assigned fejta Jun 27, 2018

cblecker requested review from cjwagner and stevekuznetsov as code owners June 27, 2018 21:52

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 27, 2018

cblecker force-pushed the nested-teams branch 2 times, most recently from ecaeb72 to 26f555f Compare June 27, 2018 22:20

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2018

fejta reviewed Jun 27, 2018

View reviewed changes

cblecker force-pushed the nested-teams branch from 26f555f to 9758247 Compare June 27, 2018 23:18

fejta reviewed Jun 28, 2018

View reviewed changes

cblecker force-pushed the nested-teams branch 3 times, most recently from 1941437 to f3330aa Compare June 29, 2018 21:44

fejta reviewed Jun 29, 2018

View reviewed changes

fejta reviewed Jun 30, 2018

View reviewed changes

cblecker force-pushed the nested-teams branch from 4234d64 to 64acb75 Compare June 30, 2018 03:28

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2018

fejta approved these changes Jun 30, 2018

View reviewed changes

cblecker force-pushed the nested-teams branch from 64acb75 to 3fd914a Compare June 30, 2018 05:38

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2018

cblecker changed the title ~~[WIP] Add support for GitHub Nested Teams~~ Add support for GitHub Nested Teams 🐣 Jun 30, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2018

cblecker force-pushed the nested-teams branch from 3fd914a to 7f86b7a Compare June 30, 2018 05:46

Add support for GitHub Nested Teams

88b98eb

cblecker force-pushed the nested-teams branch from 7f86b7a to 88b98eb Compare June 30, 2018 18:45

cblecker commented Jun 30, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2018

fejta reviewed Jun 30, 2018

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2018

k8s-ci-robot merged commit f47f634 into kubernetes:master Jun 30, 2018

cblecker deleted the nested-teams branch July 1, 2018 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GitHub Nested Teams 🐣 #8486

Add support for GitHub Nested Teams 🐣 #8486

cblecker commented Jun 27, 2018 •

edited

Loading

fejta Jun 27, 2018

fejta Jun 27, 2018

fejta Jun 27, 2018

fejta Jun 27, 2018

fejta left a comment •

edited

Loading

fejta Jun 28, 2018

cblecker commented Jun 29, 2018

fejta left a comment

fejta Jun 29, 2018

cblecker Jun 30, 2018

fejta Jun 29, 2018

fejta Jun 29, 2018

cblecker Jun 30, 2018

cblecker commented Jun 30, 2018

fejta Jun 30, 2018

cblecker Jun 30, 2018

fejta Jun 30, 2018

fejta Jun 30, 2018

cblecker Jun 30, 2018 •

edited

Loading

fejta Jun 30, 2018

fejta Jun 30, 2018

fejta Jun 30, 2018

cblecker commented Jun 30, 2018

fejta commented Jun 30, 2018

fejta left a comment

cblecker commented Jun 30, 2018

BenTheElder commented Jun 30, 2018

BenTheElder commented Jun 30, 2018 via email

cblecker Jun 30, 2018

BenTheElder commented Jun 30, 2018

fejta left a comment

k8s-ci-robot commented Jun 30, 2018

cblecker commented Jun 30, 2018

fejta commented Jun 30, 2018

Add support for GitHub Nested Teams 🐣 #8486

Add support for GitHub Nested Teams 🐣 #8486

Conversation

cblecker commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker commented Jun 29, 2018

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker commented Jun 30, 2018

fejta commented Jun 30, 2018

fejta left a comment

Choose a reason for hiding this comment

cblecker commented Jun 30, 2018

BenTheElder commented Jun 30, 2018

BenTheElder commented Jun 30, 2018 via email

Choose a reason for hiding this comment

BenTheElder commented Jun 30, 2018

fejta left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 30, 2018

cblecker commented Jun 30, 2018

fejta commented Jun 30, 2018

cblecker commented Jun 27, 2018 •

edited

Loading

fejta left a comment •

edited

Loading

cblecker Jun 30, 2018 •

edited

Loading