diff --git a/decisions/2026-08-04-ycst-admin-docs-private-cpanel.md b/decisions/2026-08-04-ycst-admin-docs-private-cpanel.md index b06ab9b..ea640c3 100644 --- a/decisions/2026-08-04-ycst-admin-docs-private-cpanel.md +++ b/decisions/2026-08-04-ycst-admin-docs-private-cpanel.md @@ -62,3 +62,9 @@ repos: `vulnerability_alerts` and `dependabot_security_updates`. `decisions/2026-07-15-homelab-docs-pages-repo.md`, which reached the opposite conclusion (public + Pages) from the same free-tier constraint, because that content was public-facing and this content is not. + +## Update 2026-08-13 + +The repo moved to `ycst-org-uk/board-docs`; admin is now granted through the +`admins` team rather than a named collaborator. The private-visibility reasoning +here is unchanged. See `decisions/2026-08-13-ycst-org-uk-migration.md`. diff --git a/decisions/2026-08-13-ycst-org-uk-migration.md b/decisions/2026-08-13-ycst-org-uk-migration.md new file mode 100644 index 0000000..1244d65 --- /dev/null +++ b/decisions/2026-08-13-ycst-org-uk-migration.md @@ -0,0 +1,121 @@ +# Decision: manage `ycst-org-uk` as a second org, with team-based admin + +`ycst-org-uk` is managed from this repository as a second org: one provider +alias, one `modules/org` call, one `data/` directory. Its repos grant admin +through an `admins` team rather than named user collaborators. + +The two York City Supporters Trust repos moved out of `yo61`: + +| Before | After | +| --- | --- | +| `yo61/ycst-admin-docs` | `ycst-org-uk/board-docs` | +| `yo61/ycst-website-testing` | `ycst-org-uk/website-testing` | +| `PlanetSeth` named as a user collaborator | `admins` team holds admin on both | + +The repos were transferred and renamed on GitHub out of band, then re-adopted +into state with `terraform state rm` plus `import` blocks — **not** with the +`moved` blocks the design specified. See "What went wrong" below. + +## Context + +Trust assets were living in a personal org, with a second trustee named as an +individual collaborator. `ycst-org-uk` was created 2026-08-12 on the free plan. +This repository was designed for a second org from the start: +`docs/dev/2026-05-22-multi-org-github-repos-design.md` reserved +leading-underscore filenames for per-org metadata and kept `collaborators.teams` +in `modules/github-repo` while leaving teams out of scope. + +Design: `docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md`. +Plan: `docs/superpowers/plans/2026-08-13-ycst-org-uk-migration.md`. +Delivered as PRs #64, #65, #66, #67. + +## Alternatives considered + +- **A fully declarative forget-and-reimport using `removed` blocks.** Rejected + by experiment: `removed` rejects instance keys — *"Module address must be a + module, not a module instance"* — so two of `yo61`'s repos cannot be forgotten + without forgetting all of `module.repo`. +- **Destroy and recreate.** Loses issues, secrets, environments, and history. +- **Separate Stategraph state per org.** Real blast-radius isolation, but would + mean migrating `yo61`'s existing instances into a new state for no benefit + currently required. Reconsider if `ycst-org-uk` gains contributors who should + not be able to apply against `yo61`. +- **Keeping named-user collaborators.** Does not scale past one trustee, and + makes offboarding a per-repo edit. +- **Renaming the repos back so the stale IDs resolved,** then letting Terraform + perform the rename itself. Avoids state surgery, but adds two more + outward-facing renames and relies on unverified provider behaviour for the + dependent alert resources, whose IDs are also the repo name. + +## Reasoning + +Team grants scale past one trustee and make membership a single edit in +`_teams.yaml`. Actions minutes and storage now bill against the trust's own +free-plan allowance rather than `yo61`'s. + +Passing `team_ids` (IDs, not slugs) from `modules/org` into `modules/github-repo` +is what creates the dependency edge ordering team creation before a repo grant. +The edge comes from referencing `github_team` at all, not from which attribute +is read or whether the `lookup` default fires. + +## What went wrong + +The design established by experiment that `moved` works across provider aliases +and renamed `for_each` keys in a single block, and that finding held. What the +experiment could not show is that **`moved` cannot carry a rename when the +resource ID is the name**, because its fixture used a resource whose ID was a +random string, stable across the move. + +For `github_repository` the ID *is* the repo name. After the `moved` blocks +rebound each resource to the `ycst_org_uk` provider, refresh asked GitHub for +`ycst-org-uk/ycst-admin-docs` and got 404: the rename redirect is keyed on the +original owner/name pair (`yo61/ycst-admin-docs`), not on the old name under the +new owner. Terraform read the 404 as "the resource is gone" and planned to +create two repos that already existed. + +The plan's gate caught it — zero destroys, but four creates where eight moves +were expected — and nothing was applied. Recovery was `terraform state rm` for +the eight stale instances, which does not touch GitHub, followed by `import` +blocks adopting the same objects at their new addresses under their new names. +Both work against the HTTP backend; the note that Stategraph lacked `state mv` +and `state rm` described the retired `stategraph tf` wrapper, not the native CLI. + +Applied result: 8 imported, 0 added, 3 changed, 0 destroyed. + +Two smaller corrections came out of the same work, both recorded in +`decisions/2026-08-13-team-member-roles.md`: GitHub makes a team's creator its +maintainer, which an authoritative `github_team_members` fights unless the role +is declared; and `github_team_members` lowercases usernames into state, so mixed +case is a standing diff. + +## Trade-offs accepted + +- **One Stategraph state still spans both orgs.** `task plan ORG=` scopes a + plan but only warns — it does not isolate. Targeting also skips the excluded + org's `check "unmanaged_repos"` and its filename/`name:` validation, so it is + for scoping a known change, not routine planning. +- **`board-docs` loses `yo61-lastlight`.** GitHub App installations do not + transfer and none were reinstalled on `ycst-org-uk`. The manual-review policy + in `decisions/2026-08-10-private-repos-manual-review-gate.md` survives — it + made human review the control precisely because rulesets are paywalled — but + the reviewer is human-only rather than bot-assisted. +- **`yo61`'s `owners` and `ubnt` teams stay unmanaged.** An org with no + `_teams.yaml` manages no teams, so they are neither adopted nor destroyed. + `owners` is a GitHub built-in that should stay that way. +- **The free-plan paywall is unchanged.** Both repos keep + `builtin_ruleset_names: []`; rulesets and classic branch protection 403 on + private repos, so neither gains an enforced merge gate. +- **`website-testing` carries `auto_init: true` against an imported `false`.** + `auto_init` is create-only and the API never reports it, so import reads + `false` and the declared `true` was applied as a state-only correction. The + design kept the field on the grounds that it was already in state; after + re-import that rationale is inverted, and dropping it would now be the + no-diff choice. + +## Supersedes + +Supersedes no prior record. Amends +`decisions/2026-08-04-ycst-admin-docs-private-cpanel.md` only in that the repo +now lives at `ycst-org-uk/board-docs`; that record's private-visibility +reasoning is unchanged. `decisions/2026-08-13-team-member-roles.md` amends this +migration's design decision on team member roles. diff --git a/docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md b/docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md index 3c8dddd..c60bb92 100644 --- a/docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md +++ b/docs/superpowers/specs/2026-08-12-ycst-org-uk-migration-design.md @@ -1,7 +1,10 @@ # `ycst-org-uk` — second managed org, team-based admin, repo migration Date: 2026-08-12 -Status: Design, pending implementation plan +Status: Implemented 2026-08-13. See `decisions/2026-08-13-ycst-org-uk-migration.md`. +Finding 1 below is incomplete: `moved` cannot carry a **rename**, because +`github_repository`'s ID is the repo name. The migration recovered with +`terraform state rm` plus `import` blocks. The decision record has the detail. ## Goal diff --git a/imports.tf b/imports.tf deleted file mode 100644 index d5bf114..0000000 --- a/imports.tf +++ /dev/null @@ -1,55 +0,0 @@ -# Temporary: re-adopts the two repos transferred from yo61 to ycst-org-uk on -# 2026-08-13. Delete once applied. -# -# `moved` blocks were the intended mechanism and half worked: they rebound each -# resource to the ycst_org_uk provider and the renamed for_each key, exactly as -# the migration design's experiment predicted. What they cannot do is rewrite a -# resource ID, and for github_repository the ID *is* the repo name. State held -# `ycst-admin-docs`, so refresh asked GitHub for ycst-org-uk/ycst-admin-docs and -# got 404 — the rename redirect is keyed on the original owner/name pair -# (yo61/ycst-admin-docs), not on the old name under the new owner. Terraform -# read the 404 as "resource is gone" and planned to create it. -# -# The stale entries were dropped with `terraform state rm` (GitHub untouched); -# these blocks adopt the same objects at their new addresses under their new -# names. `removed` blocks cannot do the dropping — they reject instance keys. - -import { - to = module.org_ycst_org_uk.module.repo["board-docs"].github_repository.this - id = "board-docs" -} - -import { - to = module.org_ycst_org_uk.module.repo["board-docs"].github_repository_collaborators.this - id = "board-docs" -} - -import { - to = module.org_ycst_org_uk.module.repo["board-docs"].github_repository_vulnerability_alerts.this["this"] - id = "board-docs" -} - -import { - to = module.org_ycst_org_uk.module.repo["board-docs"].github_repository_dependabot_security_updates.this["this"] - id = "board-docs" -} - -import { - to = module.org_ycst_org_uk.module.repo["website-testing"].github_repository.this - id = "website-testing" -} - -import { - to = module.org_ycst_org_uk.module.repo["website-testing"].github_repository_collaborators.this - id = "website-testing" -} - -import { - to = module.org_ycst_org_uk.module.repo["website-testing"].github_repository_vulnerability_alerts.this["this"] - id = "website-testing" -} - -import { - to = module.org_ycst_org_uk.module.repo["website-testing"].github_repository_dependabot_security_updates.this["this"] - id = "website-testing" -} diff --git a/quality/criteria.md b/quality/criteria.md index 3beb1cd..fef9a05 100644 --- a/quality/criteria.md +++ b/quality/criteria.md @@ -110,12 +110,23 @@ deleted. - Run `task plan` and read the diff before `task apply`. Use the `Taskfile` wrappers, not the underlying CLI. - - A plan proposing to *create* resources that already exist means the - backend is misconfigured, not that the infrastructure is missing. - Stop and fix the backend; do not apply. - - An instance address change produces destroy+create. Stategraph ignores - HCL `moved`/`removed` blocks and has no `state mv`/`rm`. Confirm that - is intended before applying. + - A plan proposing to *create* resources that already exist means either + the backend is misconfigured or a resource's ID no longer resolves. + Both look identical in the plan. Check the backend first, then check + whether refresh can still reach the object under the ID in state. Do + not apply either way. + - An instance address change needs a `moved` block, and its plan must + show the move rather than a destroy+create. Under the native CLI + against the HTTP backend `moved` is honoured, including across + provider aliases and renamed `for_each` keys in one block. The + retired `stategraph tf` wrapper ignored it. + - A `moved` block cannot carry a **rename** when the resource ID is the + name — as it is for `github_repository` and everything keyed on it. + Terraform rewrites the address and the provider binding but never the + ID, so refresh looks for the old name under the new owner, 404s, and + plans a create. Use `terraform state rm` plus `import` blocks instead; + both work against the HTTP backend. `removed` blocks cannot substitute + for the `state rm` — they reject instance keys. - Plan files may contain sensitive values and are gitignored. Never commit one. - Never echo a credential to verify it is set. Test with `${VAR:+set}`, @@ -125,10 +136,23 @@ deleted. ## Severity: blocking ## Source: `CLAUDE.md`; Stategraph state-mutation gaps; -`decisions/2026-08-04-native-terraform-http-backend.md` - -## Last triggered: 2026-08-04 — a gitignored backend file meant a clone -without it would silently use local state and plan to recreate all 130 +`decisions/2026-08-04-native-terraform-http-backend.md`; +`decisions/2026-08-13-ycst-org-uk-migration.md` + +## Last triggered: 2026-08-13 — the `ycst-org-uk` migration. `moved` blocks +rebound both repos to the new provider but left the old names as IDs, so the +plan proposed to create two repos that already existed. The create-vs-exists +criterion caught it and nothing was applied; the backend was fine, which is +why that criterion now names the second cause. Recovered with +`terraform state rm` plus `import` blocks. Also 2026-08-13, second trigger of +the credential criterion: `${GITHUB_TOKEN:+yes}${GITHUB_TOKEN:-no}` printed a +PAT in full — the `:+` guard was written correctly and then undone by a `:-` +fallback on the same line. Token rotated. One more trigger promotes it to an +automated check; consider a hook matching `\$\{[A-Z_]*(TOKEN|PASSWORD|KEY| +SECRET)[A-Z_]*:-`. + +## Last triggered (prior): 2026-08-04 — a gitignored backend file meant a +clone without it would silently use local state and plan to recreate all 130 instances; and `TF_HTTP_PASSWORD` was printed in full by a `${VAR:-}` check. ---