Support parent (nested) teams

.schemas/teams-config.schema.json

@@ -25,6 +25,10 @@
         },
         "notifications": {
           "type": "boolean"
+        },
+        "parent": {
+          "type": "string",
+          "description": "Name of the parent team (must be another team defined in teams.yaml). Omit for a top-level team."
         }
       },
       "additionalProperties": false,

docs/changing-team-nesting.md

@@ -0,0 +1,44 @@
+> [!IMPORTANT]
+> Adding or removing a team's `parent` (making a top-level team nested, or a nested team top-level) is **not a normal config change**. Editing the YAML alone destroys and recreates the team on GitHub. Follow this runbook.
+>
+> Note: only **child ↔ root** transitions need this. Changing *which* parent a nested team has (parent A → parent B) is a normal in-place update — just edit the YAML.
+
+## Why
+
+Teams are managed by two resources so a child can reference its parent for correct create ordering (a single `for_each` self-reference cycles):
+
+| Team has a `parent`? | Resource |
+|---|---|
+| no (top-level) | `github_team.team["<name>"]` |
+| yes (nested) | `github_team.child_team["<name>"]` |
+
+Adding a `parent` moves the team `github_team.team → github_team.child_team`; removing it moves it back. Terraform sees the old address disappear and a new one appear, so it plans a **destroy + create**. The destroy is real: **deleting a GitHub team removes its members and every repository grant.**
+
+The fix: **move the state entry to the new address first, then apply** — so Terraform sees the same resource and does an in-place `parent_team_id` update instead.
+
+## Steps
+
+Prereq: an HCP Terraform token for the config repo's workspace (`state mv` makes no GitHub API calls — no App/PEM creds, no `-var-file`).
+
+```bash
+git clone https://github.com/G-Research/github-terraformer.git && cd github-terraformer/feature/github-repo-provisioning
+ln -sf backend.tf.hcp backend.tf
+export TF_CLOUD_ORGANIZATION=<TFC_ORG> TF_WORKSPACE=<WORKSPACE>   # config repo's tfc_org input and WORKSPACE variable
+terraform init -input=false
+terraform state pull > backup.tfstate   # always back up first
+```
+
+1. **Open a PR** editing `organisation/teams.yaml` — add or remove the team's `parent:` — and get it approved. Its first plan shows `1 to add, 1 to destroy` for the team (`github_team.child_team["<name>"]` + `github_team.team["<name>"]`) — **do not merge**.
+2. **Move the state entry** to the new address (nothing changes on GitHub):
+   ```bash
+   # adding a parent (top-level -> nested):
+   terraform state mv 'github_team.team["<name>"]' 'github_team.child_team["<name>"]'
+   # removing a parent (nested -> top-level): reverse the two addresses
+   ```
+3. **Re-plan and merge.** The plan is now `0 to destroy` — a single in-place `parent_team_id` change. Merge; the apply just re-parents (or un-parents) the team, membership and repo grants intact.
+
+> [!CAUTION]
+> Between the `state mv` and the merge, state no longer matches `main`. Any other config PR merging in that window plans a destroy+create for this team. Approve first, then `state mv`, then merge immediately (freeze merges if the org is busy).
+
+> [!NOTE]
+> **One level of nesting only.** A team's `parent` must be a top-level team. Deleting a parent that still has children, or nesting under a nested team, fails the plan with `Error: Invalid index` — detach/reparent the children first.

feature/github-repo-importer/pkg/github/org.go

@@ -32,9 +32,6 @@ func ImportOrg(org string) (*TeamsConfig, *MembersConfig, error) {
 	if err != nil {
 		return nil, nil, err
 	}
-	if err := rejectNestedTeams(ghTeams); err != nil {
-		return nil, nil, err
-	}
 
 	memberLogins, err := listMemberLogins(ctx, org, "all")
 	if err != nil {
@@ -80,15 +77,6 @@ func listAllTeams(ctx context.Context, org string) ([]*orgTeam, error) {
 	return all, nil
 }
 
-func rejectNestedTeams(teams []*orgTeam) error {
-	for _, t := range teams {
-		if t.Parent != nil {
-			return fmt.Errorf("team %q has parent team %q: nested teams are not supported, cannot import", t.GetName(), t.GetParent().GetName())
-		}
-	}
-	return nil
-}
-
 func buildTeamsConfig(ghTeams []*orgTeam) (*TeamsConfig, error) {
 	teams := make([]Team, 0, len(ghTeams))
 	for _, t := range ghTeams {
@@ -104,6 +92,11 @@ func buildTeamsConfig(ghTeams []*orgTeam) (*TeamsConfig, error) {
 			team.Description = &d
 		}
 
+		if parent := t.GetParent(); parent != nil {
+			p := parent.GetName()
+			team.Parent = &p
+		}
+
 		if t.GetPrivacy() == "secret" {
 			team.Visibility = TeamVisibilitySecret
 		} else {
@@ -163,21 +156,39 @@ func fetchTeamRosters(ctx context.Context, org string, ghTeams []*orgTeam) ([]te
 	return rosters, nil
 }
 
+// teamMember is a team member as returned by GET orgs/{org}/teams/{slug}/members.
+// go-github v67 does not map the "inherited" flag, so we decode it ourselves: the
+// endpoint returns members of child teams too (GitHub: "Team members will include the
+// members of child teams"), and those must not be recorded as direct members of the parent.
+type teamMember struct {
+	github.User
+	Inherited bool `json:"inherited"`
+}
+
 func listTeamMemberLogins(ctx context.Context, org, teamSlug, role string) ([]string, error) {
 	var logins []string
-	opts := &github.TeamListTeamMembersOptions{Role: role, ListOptions: github.ListOptions{PerPage: DefaultPageSize}}
+	page := 1
 	for {
-		users, resp, err := v3client.Teams.ListTeamMembersBySlug(ctx, org, teamSlug, opts)
+		req, err := v3client.NewRequest("GET", fmt.Sprintf("orgs/%s/teams/%s/members?role=%s&per_page=%d&page=%d", org, teamSlug, role, DefaultPageSize, page), nil)
+		if err != nil {
+			return nil, fmt.Errorf("failed to build team members request for %q: %w", teamSlug, err)
+		}
+		var members []teamMember
+		resp, err := v3client.Do(ctx, req, &members)
 		if err != nil {
 			return nil, fmt.Errorf("failed to list %s of team %q: %w", role, teamSlug, err)
 		}
-		for _, u := range users {
-			logins = append(logins, u.GetLogin())
+		for _, m := range members {
+			if m.Inherited {
+				// Inherited from a child team — not a direct member of this team.
+				continue
+			}
+			logins = append(logins, m.GetLogin())
 		}
 		if resp.NextPage == 0 {
 			break
 		}
-		opts.Page = resp.NextPage
+		page = resp.NextPage
 	}
 	return logins, nil
 }

feature/github-repo-importer/pkg/github/org_test.go

@@ -28,19 +28,19 @@ func TestOrgTeamDecode(t *testing.T) {
 	assert.Equal(t, "My_Team", teams[1].GetParent().GetName())
 }
 
-func TestRejectNestedTeams(t *testing.T) {
-	flat := []*orgTeam{
-		{Team: github.Team{Name: github.String("platform")}},
-		{Team: github.Team{Name: github.String("security")}},
-	}
-	assert.NoError(t, rejectNestedTeams(flat))
-
-	nested := []*orgTeam{
-		{Team: github.Team{Name: github.String("platform")}},
-		{Team: github.Team{Name: github.String("oncall"), Parent: &github.Team{Name: github.String("platform")}}},
+func TestBuildTeamsConfigCapturesParent(t *testing.T) {
+	ghTeams := []*orgTeam{
+		{Team: github.Team{Name: github.String("platform"), Slug: github.String("platform"), Privacy: github.String("closed")}, NotificationSetting: NotificationsEnabled},
+		{Team: github.Team{Name: github.String("oncall"), Slug: github.String("oncall"), Privacy: github.String("closed"), Parent: &github.Team{Name: github.String("platform")}}, NotificationSetting: NotificationsEnabled},
 	}
-	err := rejectNestedTeams(nested)
-	assert.EqualError(t, err, `team "oncall" has parent team "platform": nested teams are not supported, cannot import`)
+	cfg, err := buildTeamsConfig(ghTeams)
+	assert.NoError(t, err)
+	assert.Len(t, cfg.Teams, 2)
+	// buildTeamsConfig sorts by name: oncall, platform
+	assert.Equal(t, "oncall", cfg.Teams[0].Name)
+	assert.NotNil(t, cfg.Teams[0].Parent)
+	assert.Equal(t, "platform", *cfg.Teams[0].Parent)
+	assert.Nil(t, cfg.Teams[1].Parent)
 }
 
 func TestBuildTeamsConfig(t *testing.T) {
@@ -137,3 +137,29 @@ func TestBuildMembersConfigEmptyOrg(t *testing.T) {
 	config := buildMembersConfig(nil, nil, nil)
 	assert.Empty(t, config.Members)
 }
+
+func TestTeamMemberDecodeSkipsInherited(t *testing.T) {
+	// GET orgs/{org}/teams/{slug}/members returns inherited (child-team) members too.
+	payload := `[
+		{"login":"direct-user","inherited":false},
+		{"login":"child-user","inherited":true}
+	]`
+
+	var members []teamMember
+	err := json.Unmarshal([]byte(payload), &members)
+	assert.NoError(t, err)
+	assert.Len(t, members, 2)
+	assert.Equal(t, "direct-user", members[0].GetLogin())
+	assert.False(t, members[0].Inherited)
+	assert.True(t, members[1].Inherited)
+
+	// Only direct members belong in a parent team's roster.
+	var kept []string
+	for _, m := range members {
+		if m.Inherited {
+			continue
+		}
+		kept = append(kept, m.GetLogin())
+	}
+	assert.Equal(t, []string{"direct-user"}, kept)
+}

feature/github-repo-importer/pkg/github/teams.go

@@ -15,6 +15,7 @@ type Team struct {
 	Description   *string `yaml:"description,omitempty"`
 	Visibility    string  `yaml:"visibility,omitempty" jsonschema:"enum=visible,enum=secret"`
 	Notifications *bool   `yaml:"notifications,omitempty"`
+	Parent        *string `yaml:"parent,omitempty" jsonschema:"description=Name of the parent team (must be another team defined in teams.yaml). Omit for a top-level team."`
 }
 
 func (c *TeamsConfig) Validate() []error {
@@ -34,5 +35,29 @@ func (c *TeamsConfig) Validate() []error {
 		}
 	}
 
+	// Validate parent (nested team) references.
+	parentOf := make(map[string]*string, len(c.Teams))
+	for _, team := range c.Teams {
+		parentOf[team.Name] = team.Parent
+	}
+	for _, team := range c.Teams {
+		if team.Parent == nil {
+			continue
+		}
+		parent := *team.Parent
+		parentsParent, parentIsDefined := parentOf[parent]
+		switch {
+		case parent == team.Name:
+			// Rule 3: a team cannot be its own parent.
+			errs = append(errs, fmt.Errorf("team %q cannot be its own parent", team.Name))
+		case !parentIsDefined:
+			// Rule 1: parent must be a team defined in teams.yaml.
+			errs = append(errs, fmt.Errorf("team %q has parent %q which is not defined in teams.yaml", team.Name, parent))
+		case parentsParent != nil:
+			// Rule 2: the parent must itself be top-level — only one level of nesting is supported.
+			errs = append(errs, fmt.Errorf("team %q nests under %q, which is itself nested under %q; only one level of team nesting is supported", team.Name, parent, *parentsParent))
+		}
+	}
+
 	return errs
 }

feature/github-repo-importer/pkg/github/teams_test.go

@@ -50,6 +50,51 @@ func TestTeamsConfigValidate(t *testing.T) {
 				`team "security-core" is defined more than once in teams.yaml`,
 			},
 		},
+		{
+			name: "valid one-level nesting",
+			config: TeamsConfig{
+				Teams: []Team{
+					{Name: "platform", Visibility: TeamVisibilityVisible},
+					{Name: "platform-oncall", Visibility: TeamVisibilityVisible, Parent: strptr("platform")},
+				},
+			},
+			wantErrors: nil,
+		},
+		{
+			name: "parent not defined rejected",
+			config: TeamsConfig{
+				Teams: []Team{
+					{Name: "child", Visibility: TeamVisibilityVisible, Parent: strptr("ghost")},
+				},
+			},
+			wantErrors: []string{
+				`team "child" has parent "ghost" which is not defined in teams.yaml`,
+			},
+		},
+		{
+			name: "multi-level nesting rejected",
+			config: TeamsConfig{
+				Teams: []Team{
+					{Name: "gp", Visibility: TeamVisibilityVisible},
+					{Name: "mid", Visibility: TeamVisibilityVisible, Parent: strptr("gp")},
+					{Name: "leaf", Visibility: TeamVisibilityVisible, Parent: strptr("mid")},
+				},
+			},
+			wantErrors: []string{
+				`team "leaf" nests under "mid", which is itself nested under "gp"; only one level of team nesting is supported`,
+			},
+		},
+		{
+			name: "self-parent rejected",
+			config: TeamsConfig{
+				Teams: []Team{
+					{Name: "selfie", Visibility: TeamVisibilityVisible, Parent: strptr("selfie")},
+				},
+			},
+			wantErrors: []string{
+				`team "selfie" cannot be its own parent`,
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -64,3 +109,5 @@ func TestTeamsConfigValidate(t *testing.T) {
 		})
 	}
 }
+
+func strptr(s string) *string { return &s }

feature/github-repo-provisioning/members.tf

@@ -60,7 +60,7 @@ resource "github_membership" "member" {
 resource "github_team_membership" "membership" {
   for_each = local.member_team_pairs
 
-  team_id  = github_team.team[each.value.team].id
+  team_id  = local.team_ids[each.value.team]
   username = each.value.username
   role     = each.value.role
 }

feature/github-repo-provisioning/teams.tf

@@ -11,20 +11,56 @@ locals {
     local.staged_teams_by_name,
     { for t in try(local.teams_raw.teams, []) : t.name => t },
   )
+
+  # Split teams by whether they nest under a parent. Two resources let a child reference its
+  # (root) parent's id for correct create ordering, which a single for_each resource can't do
+  # (a self-reference across instances cycles). Supports one level of nesting.
+  root_teams_by_name  = { for k, t in local.teams_by_name : k => t if try(t.parent, null) == null }
+  child_teams_by_name = { for k, t in local.teams_by_name : k => t if try(t.parent, null) != null }
+  # Bucket staged teams by the MERGED result (final config wins), not the staged file's own
+  # parent — otherwise, when a team's parent differs between staged and final (e.g. re-import
+  # after a parent change on GitHub), the import target address wouldn't match the resource.
+  staged_root_teams  = { for k, t in local.staged_teams_by_name : k => t if contains(keys(local.root_teams_by_name), k) }
+  staged_child_teams = { for k, t in local.staged_teams_by_name : k => t if contains(keys(local.child_teams_by_name), k) }
+
+  # Team name -> id across both resources, for github_team_membership and any other lookups.
+  team_ids = merge(
+    { for k, t in github_team.team : k => t.id },
+    { for k, t in github_team.child_team : k => t.id },
+  )
 }
 
 import {
-  for_each = local.staged_teams_by_name
+  for_each = local.staged_root_teams
 
   to = github_team.team[each.key]
   id = try(each.value.slug, each.key)
 }
 
+import {
+  for_each = local.staged_child_teams
+
+  to = github_team.child_team[each.key]
+  id = try(each.value.slug, each.key)
+}
+
 resource "github_team" "team" {
-  for_each = local.teams_by_name
+  for_each = local.root_teams_by_name
+
+  name                 = each.value.name
+  description          = try(each.value.description, null)
+  privacy              = try(each.value.visibility, "visible") == "secret" ? "secret" : "closed"
+  notification_setting = coalesce(try(each.value.notifications, true), true) ? "notifications_enabled" : "notifications_disabled"
+}
+
+resource "github_team" "child_team" {
+  for_each = local.child_teams_by_name
 
   name                 = each.value.name
   description          = try(each.value.description, null)
   privacy              = try(each.value.visibility, "visible") == "secret" ? "secret" : "closed"
   notification_setting = coalesce(try(each.value.notifications, true), true) ? "notifications_enabled" : "notifications_disabled"
+  # Parent referenced by name; it must be a root team (github_team.team) — one level of nesting.
+  # The reference gives Terraform the parent-before-child ordering without a self-cycle.
+  parent_team_id = github_team.team[each.value.parent].id
 }