diff --git a/CLAUDE.md b/CLAUDE.md index baf8668..aa11f99 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,13 +48,21 @@ files. scanning are paywalled — omit them. Keep `vulnerability_alerts` and `dependabot_security_updates`. - **Teams are optional and per-org.** `data//_teams.yaml` is a map keyed - by team slug, each with `description`, `members`, and an optional `privacy` - (default `closed`). The key is used verbatim as the team name, and GitHub - derives the slug from it, so keys must be lowercase and hyphenated for the - two to agree. Membership is authoritative — a member added in the UI is - removed on the next apply. A repo grants to a team with + by team slug, each with `description`, `members`, `maintainers`, and an + optional `privacy` (default `closed`). The key is used verbatim as the team + name, and GitHub derives the slug from it, so keys must be lowercase and + hyphenated for the two to agree. Membership is authoritative — a member added + in the UI is removed on the next apply. A repo grants to a team with `collaborators.teams: [{permission: admin, slug: admins}]`; a slug with no team in the same org's `_teams.yaml` fails the plan. +- **List a new team's creator under `maintainers`.** GitHub makes whoever + creates a team its maintainer, so a team whose YAML lists only `members` + shows a standing diff demoting them. See + `decisions/2026-08-13-team-member-roles.md`. +- **Team usernames go in lowercase.** `github_team_members` lowercases them + into state and compares case-sensitively, so `PlanetSeth` under a team is a + standing diff. This is specific to team membership — `collaborators.users` + takes GitHub's display case and does not drift. ## Applying changes diff --git a/data/ycst-org-uk/_teams.yaml b/data/ycst-org-uk/_teams.yaml index 4c5c37f..053eb5b 100644 --- a/data/ycst-org-uk/_teams.yaml +++ b/data/ycst-org-uk/_teams.yaml @@ -1,6 +1,9 @@ --- admins: description: Administrators for York City Supporters Trust repositories - members: + maintainers: - robinbowes - - PlanetSeth + members: + # Lowercase: github_team_members lowercases usernames into state and + # compares case-sensitively, so `PlanetSeth` here is a standing diff. + - planetseth diff --git a/decisions/2026-08-13-team-member-roles.md b/decisions/2026-08-13-team-member-roles.md new file mode 100644 index 0000000..b20e79f --- /dev/null +++ b/decisions/2026-08-13-team-member-roles.md @@ -0,0 +1,92 @@ +# Decision: `_teams.yaml` carries `maintainers` alongside `members` + +`data//_teams.yaml` gains a second optional list, `maintainers`, holding +usernames granted the team's `maintainer` role. `members` keeps its meaning — +a plain list of usernames granted `member`. `modules/org/teams.tf` renders one +`dynamic "members"` block per list. + +The same change switches `github_team_members` from `team_id` to `team_slug`. + +## Context + +`docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md` decided +against roles, under "Decisions taken without further input": + +> **Flat member list, no `maintainer`/`member` roles.** The provider defaults a +> member's role to `member`, and `robinbowes` already administers the team by +> virtue of org ownership, so roles would add configuration surface for no +> behaviour. Add them when a team needs a maintainer who is not an org owner. + +PR #64 created `ycst-org-uk/admins` with that flat list. The apply reported +`3 added, 0 changed, 0 destroyed`, but the next plan was not clean: + +``` +module.org_ycst_org_uk.github_team_members.this["admins"] + robinbowes: maintainer → member +``` + +GitHub makes whoever creates a team its maintainer. `github_team_members` is +authoritative and the config named no role, so the provider default `member` +became a standing instruction to demote the creator. + +The same apply raised a provider deprecation: `team_id` is deprecated in favour +of `team_slug` and "will be made computed only in a future version". The root +module's `version = "~> 6.0"` permits that release, so the config would break +on a routine provider bump. + +## Alternatives considered + +- **Apply the demotion.** Let `robinbowes` become a plain member. Minimal + config, and the original reasoning survives — org ownership administers the + team regardless of team role. +- **One `members` list whose entries may be a string or a `{username, role}` + object.** Keeps a single list, at the cost of a heterogeneous type that + Terraform reasons about poorly and that every reader has to decode. +- **Every member an object with an explicit `role`.** Uniform, but verbose for + the common case of a team with no maintainers. +- **`lifecycle { ignore_changes }` on the members block.** Silences the diff + without expressing intent, and would mask genuine membership drift. +- **Defer the `team_slug` switch to a later PR.** Rejected: it touches the same + resource, so landing both together means one plan shows their combined effect. + +## Reasoning + +The spec's decision was sound but its stated premise — "roles would add +configuration surface for no behaviour" — turned out to be false. Omitting the +role does have a behaviour: a standing diff. Once roles must be expressed, two +parallel lists of plain usernames cost less than a heterogeneous list, and a +team with no maintainers writes exactly what it writes today. + +Declaring the reality rather than demoting the creator also keeps the YAML +honest about what GitHub shows in its UI, which is the point of an +authoritative resource. + +`team_slug = github_team.this[each.key].slug` preserves the dependency edge +that orders team creation before any repo grant. The edge comes from +referencing `github_team` at all, not from which attribute is read. + +Usernames under a team are written lowercase. `github_team_members` lowercases +them into state and compares case-sensitively, so `PlanetSeth` is a standing +diff where `planetseth` is stable. This is specific to team membership — +`collaborators.users` in a repo file carries GitHub's display case and has never +drifted, so the two are deliberately inconsistent rather than uniformly +lowercased. + +## Trade-offs accepted + +- Two lists can disagree: a username in both `maintainers` and `members` + produces two blocks for one user. No validation guards this — consistent with + declining the analogous non-empty-members precondition in PR #64, and plan + review is the control. +- `maintainers` must be maintained by hand as teams change. GitHub will not + auto-correct it, and the standing diff that revealed the problem here only + appears at team creation. +- Reverses a decision recorded one day earlier, so the spec now contains a + superseded claim. The spec is left unedited as a record of what was believed + at the time; this file is the correction. + +## Supersedes + +Amends the "Flat member list, no `maintainer`/`member` roles" decision in +`docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md`. Supersedes +no prior file in `decisions/`. diff --git a/modules/org/teams.tf b/modules/org/teams.tf index 4458484..51ea7cd 100644 --- a/modules/org/teams.tf +++ b/modules/org/teams.tf @@ -13,18 +13,35 @@ resource "github_team" "this" { } # Authoritative, matching this repo's "the YAML is the source of truth" posture: -# a member added through the GitHub UI is removed on the next apply. Role is -# left at the provider default (`member`); org owners already administer their -# own teams. +# a member added through the GitHub UI is removed on the next apply. +# +# `maintainers` and `members` are separate lists rather than one list carrying a +# role, so the common case stays a plain list of usernames. Both are needed: +# GitHub makes whoever creates a team its maintainer, so a team whose YAML lists +# only `members` shows a standing diff demoting its creator. Observed on +# ycst-org-uk/admins, 2026-08-13. +# +# team_slug rather than team_id: the provider deprecated team_id and will make +# it computed-only. Referencing github_team.this keeps the dependency edge that +# orders team creation before any repo grant either way. resource "github_team_members" "this" { for_each = local.teams - team_id = github_team.this[each.key].id + team_slug = github_team.this[each.key].slug dynamic "members" { - for_each = toset(lookup(each.value, "members", [])) + for_each = toset(coalesce(lookup(each.value, "maintainers", []), [])) content { username = members.value + role = "maintainer" + } + } + + dynamic "members" { + for_each = toset(coalesce(lookup(each.value, "members", []), [])) + content { + username = members.value + role = "member" } } }