diff --git a/.github/workflows/apply.yaml b/.github/workflows/apply.yaml index 703f7cb..5a04a2d 100644 --- a/.github/workflows/apply.yaml +++ b/.github/workflows/apply.yaml @@ -166,6 +166,53 @@ jobs: env: GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }} EXCLUDE_ADDRESSES: ${{ inputs.exclude_addresses }} + # Review assignment of the maintainers team: GitHub assigns one DIRECT member of the team (osac-ci-bot, see teams.tf) to + # a pull request that CODEOWNERS sends to the team, and notifies only that member, not the whole team. The provider + # (github_team_settings) cannot set includeChildTeamMembers, which is what leaves the members of the working-group + # teams out of the assignment, so it is set here, on every apply: the mutation is idempotent, and it puts the setting + # back if someone changed it by hand. It is not readable back (the team has no field for includeChildTeamMembers). + # It also runs after an apply that failed on something else (a broken resource must not hold the setting back), but + # only when osac-ci-bot is a direct member of the team: with nobody eligible, the assignment has no one to pick. + - name: Maintainers team review assignment + if: ${{ !cancelled() && steps.tofu_apply.outcome != 'skipped' }} + env: + GH_TOKEN: ${{ steps.generate-token.outputs.token }} + ORG: ${{ github.repository_owner }} + SINK: osac-ci-bot + run: | + set -euo pipefail + answer=$(gh api graphql -f org="${ORG}" -f query=' + query($org: String!) { + organization(login: $org) { + team(slug: "maintainers") { id members(membership: IMMEDIATE, first: 100) { nodes { login } } } + } + }') + team_id=$(jq -r '.data.organization.team.id // empty' <<<"${answer}") + if [[ -z "${team_id}" ]]; then + echo "::error::The maintainers team was not found." >&2 + exit 1 + fi + direct=$(jq -r '.data.organization.team.members.nodes | map(.login) | join(" ")' <<<"${answer}") + if ! jq -e --arg sink "${SINK}" '.data.organization.team.members.nodes | map(.login) | index($sink)' <<<"${answer}" >/dev/null; then + echo "::warning::${SINK} is not a direct member of maintainers (yet): the review assignment is left as it is." + exit 0 + fi + # Only direct members can be assigned, so the assignment stays harmless only while the bot is the ONLY one: anyone + # else added to the team directly would be picked for pull requests. + if [[ "${direct}" != "${SINK}" ]]; then + echo "::error::maintainers has direct members besides ${SINK} (${direct}): they would be assigned to pull requests. Remove them from the team (its members are the working-group teams), then run this again." >&2 + exit 1 + fi + gh api graphql -f id="${team_id}" -f query=' + mutation($id: ID!) { + updateTeamReviewAssignment(input: { + id: $id, enabled: true, algorithm: LOAD_BALANCE, teamMemberCount: 1, + notifyTeam: false, includeChildTeamMembers: false, removeTeamRequest: false + }) { team { slug reviewRequestDelegationEnabled reviewRequestDelegationMemberCount reviewRequestDelegationNotifyTeam } } + }' --jq '.data.updateTeamReviewAssignment.team' | tee "${RUNNER_TEMP}/review-assignment.json" + # What GitHub says it stored (includeChildTeamMembers is not part of the answer): fail the run if it is not what was asked. + jq -e '.reviewRequestDelegationEnabled == true and .reviewRequestDelegationMemberCount == 1 + and .reviewRequestDelegationNotifyTeam == false' "${RUNNER_TEMP}/review-assignment.json" >/dev/null # tofu apply isn't atomic and doesn't roll back changes already # applied earlier in the same run if a later resource fails -- but a # hard error on one resource does still stop the run before anything diff --git a/README.md b/README.md index cd860bf..8bc0d76 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,13 @@ The team then becomes a child of the `maintainers` team, whose members are every The point of the team is `osac`'s CODEOWNERS: a follow-up change to that file will make `@osac-project/maintainers` the owner of every path that no other rule names, so that a person in one of these teams has to approve it and a bot (CodeRabbit included) never counts. Until that change is merged, the team is only created, and paths with no owner can still be approved by a bot. +**Notifications and review assignment.** A pull request that CODEOWNERS sends to `maintainers` requests the team, and every member of a working group may approve. GitHub notifies every member of a team that is requested for review, and its team notification setting does not change that (it only covers @mentions). The only switch is *review assignment* (Code review in the team's settings): GitHub assigns members and notifies only them. So that nobody is picked and nobody is broadcast to: + +- `osac-ci-bot` is the only **direct** member of `maintainers` (`github_team_membership.maintainers_review_sink` in `teams.tf`). It approves nothing. Nobody else may be added to `maintainers` directly (its members are the working-group teams): a direct member would be assigned to pull requests, and the apply step fails when it finds one. +- Review assignment is on for `maintainers`: `LOAD_BALANCE`, one member, the team is not notified, and the members of the child teams are **left out** (`includeChildTeamMembers: false`), so the assignment always picks the bot. The pull request lists `maintainers` and `osac-ci-bot` as reviewers, and the bot's request stays pending (nothing waits for it). Approval is not affected: members of a child team count as members of `maintainers` for CODEOWNERS. +- The provider cannot set `includeChildTeamMembers` (and the team has no field to read it back), so `.github/workflows/apply.yaml` sets all of it with the GraphQL mutation `updateTeamReviewAssignment` after every apply, scheduled ones included. `github_team_settings.maintainers` is gone from the configuration (`removed` block, nothing destroyed). +- The teams that own a folder in CODEOWNERS (`wg-infra`, `wg-vmaas`, ...) are requested and notified as usual. + ### Add an organization member to a team? 1. Open `team-members/.csv` diff --git a/teams.tf b/teams.tf index 0aa444e..4c4fe5c 100644 --- a/teams.tf +++ b/teams.tf @@ -21,14 +21,26 @@ resource "github_team" "maintainers" { privacy = "closed" } -resource "github_team_settings" "maintainers" { - team_id = github_team.maintainers.id - # Team-only requests still email members; assign one reviewer so notify = false suppresses whole-team notifications. - notify = false +# The only DIRECT member of maintainers. A pull request that CODEOWNERS sends to maintainers has GitHub assign one member of +# the team to review it (review assignment, set after every apply, see .github/workflows/apply.yaml), and only that member is +# notified, not the 80-odd people of the working groups. The assignment is limited to direct members (the members of the +# child teams are left out), so it always picks this account, which approves nothing: nobody is picked, nobody is +# broadcast to, and any member of a working group may still approve (members of a child team count as members of +# maintainers for CODEOWNERS). osac-ci-bot is already a member of the organization and of wg-infra. +resource "github_team_membership" "maintainers_review_sink" { + team_id = github_team.maintainers.id + username = "osac-ci-bot" + role = "member" +} + +# Review assignment used to be managed here (github_team_settings), but the provider cannot set the option that leaves the +# child teams out of the assignment, and an apply would undo it. It is set by apply.yaml instead; forget the old resource +# without destroying it. +removed { + from = github_team_settings.maintainers - review_request_delegation { - algorithm = "LOAD_BALANCE" - member_count = 1 + lifecycle { + destroy = false } }