From 8b5281d357ae82dcc6e60e66131bf8737123aecc Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:45:49 -0400 Subject: [PATCH] ci(terraform-isolation): re-evaluate the gate on label changes (#177) * ci(terraform-isolation): re-evaluate the gate on label changes * test(terraform-isolation): lock the ci.yaml label-event contract * fix(terraform-isolation): do not treat terraform markdown as a mixed change * fix(ci): do not skip Frontend checks on isolation label events * ci(terraform-isolation): run label retriggers in a dedicated workflow * fix: apply eslint formatting * fix: apply additional missed eslint formatting --- .github/workflows/ci.yaml | 24 ---------- .github/workflows/terraform-isolation.yaml | 43 +++++++++++++++++ QUALITY_GATES.md | 18 ++++--- README.md | 8 ++-- scripts/check-terraform-isolation.mjs | 13 ++++- scripts/check-terraform-isolation.test.mjs | 56 ++++++++++++++++++++++ terraform/README.md | 16 ++++--- 7 files changed, 135 insertions(+), 43 deletions(-) create mode 100644 .github/workflows/terraform-isolation.yaml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 53262ad1..9a39ad3b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -71,30 +71,6 @@ jobs: env: GOVERNANCE_BASE: ${{ steps.governance-ref.outputs.base }} - terraform-isolation: - # Fails a pull request that changes `terraform/**` together with deployable - # application code (scripts/check-terraform-isolation.mjs). A merge that - # does both queues an HCP VCS run and a content release at the same time, - # and the two race for the workspace lock. The - # `terraform-isolation-override` label is the reviewed exception; it is - # read when the job runs, so re-run this workflow after labeling. - name: Terraform and application changes are isolated - if: github.event_name == 'pull_request' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - fetch-depth: 0 - - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 - with: - node-version: "24" - - name: Check changed files - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - TERRAFORM_ISOLATION_OVERRIDE: ${{ contains(github.event.pull_request.labels.*.name, 'terraform-isolation-override') }} - run: node scripts/check-terraform-isolation.mjs --base "${BASE_SHA}" --head "${HEAD_SHA}" - visual-regression: name: Visual regression runs-on: ubuntu-latest diff --git a/.github/workflows/terraform-isolation.yaml b/.github/workflows/terraform-isolation.yaml new file mode 100644 index 00000000..92cc9d38 --- /dev/null +++ b/.github/workflows/terraform-isolation.yaml @@ -0,0 +1,43 @@ +name: Terraform isolation + +# Own workflow so labeled/unlabeled re-evaluate this gate without starting a +# new Frontend checks run. Skipping jobs inside `ci.yaml` on those events +# would report required checks as success and could merge a failing SHA. + +on: + pull_request: + branches: [main, dev, staging] + types: + - opened + - synchronize + - reopened + - labeled + - unlabeled + +permissions: + contents: read + +jobs: + terraform-isolation: + # Fails a pull request that changes Terraform infrastructure together with + # deployable application code (scripts/check-terraform-isolation.mjs). A + # merge that does both queues an HCP VCS run and a content release at the + # same time, and the two race for the workspace lock. The + # `terraform-isolation-override` label is the reviewed exception. This + # job is unconditional so adding or removing that label always reads the + # current label set; a previous green check does not survive removal. + name: Terraform and application changes are isolated + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: "24" + - name: Check changed files + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + TERRAFORM_ISOLATION_OVERRIDE: ${{ contains(github.event.pull_request.labels.*.name, 'terraform-isolation-override') }} + run: node scripts/check-terraform-isolation.mjs --base "${BASE_SHA}" --head "${HEAD_SHA}" diff --git a/QUALITY_GATES.md b/QUALITY_GATES.md index 60ed9970..64d5292e 100644 --- a/QUALITY_GATES.md +++ b/QUALITY_GATES.md @@ -30,7 +30,7 @@ and synthesis. A task is not done until this is green. | Terraform isolation gate contract | `npm run test:terraform-isolation` → `scripts/check-terraform-isolation.test.mjs` | `governance` + CI | Changed-file classifier | | Terraform formatting/validation | `npm run test:terraform` → `scripts/terraform-validate.mjs` | `governance` + CI | `terraform/live/dev` | | CDK build, tests, synthesis | `npm run test:infra` | `governance` + CI | `infra/cdk/**`, both synth modes | -| Terraform/app change isolation | `ci.yaml` job `terraform-isolation` → `scripts/check-terraform-isolation.mjs` | CI (PR) | Changed files of the PR | +| Terraform/app change isolation | `terraform-isolation.yaml` job `terraform-isolation` → `scripts/check-terraform-isolation.mjs` | CI (PR) | Changed files of the PR | ## No-false-pass guarantees @@ -49,10 +49,13 @@ and synthesis. A task is not done until this is green. against synthetic plan JSON. Real import and controlled-update plans from HCP are migration evidence reviewed by a human before an approved apply (`terraform/README.md`). -- **The isolation gate reads the override when it runs** — the +- **The isolation gate re-evaluates on label changes** — the `terraform-isolation-override` label is the only way to merge a mixed - Terraform/application PR, the gate logs the override on the run, and the - workflow must be re-run after the label is added or removed. + Terraform/application PR. `.github/workflows/terraform-isolation.yaml` + runs `terraform-isolation` on `labeled` and `unlabeled` as well as the + default pull-request types, so adding or removing the label re-checks + the current labels without starting a new Frontend checks run. Removing + the label fails a mixed PR that had previously passed with the override. ## Where the gates run @@ -63,9 +66,10 @@ and synthesis. A task is not done until this is green. format/lint/build/tests, **and** a repo-owned `governance` job runs `npm run verify` (with Terraform 1.16.0 installed) so the maintainability ratchets and repository gates are guaranteed from this repository regardless - of the reusable workflow. On pull requests the same workflow's - `terraform-isolation` job fails when `terraform/**` and application code - change together. + of the reusable workflow. +- **Terraform isolation ([`.github/workflows/terraform-isolation.yaml`](.github/workflows/terraform-isolation.yaml)):** + on pull requests, fails when Terraform infrastructure and application code + change together. Label add/remove re-runs only this workflow. ## Toolchain pin diff --git a/README.md b/README.md index 9a4bc04a..1abeac15 100644 --- a/README.md +++ b/README.md @@ -156,10 +156,10 @@ No stored AWS keys — OIDC only. Infrastructure and content deploy separately: [`AGENTS.md`](AGENTS.md), [`QUALITY_GATES.md`](QUALITY_GATES.md), [`ARCHITECTURE_AND_CODE_QUALITY.md`](ARCHITECTURE_AND_CODE_QUALITY.md), and [`REVIEW_AND_PR_FRAMEWORK.md`](REVIEW_AND_PR_FRAMEWORK.md). -- **Terraform isolation** (the `terraform-isolation` job in - [`.github/workflows/ci.yaml`](.github/workflows/ci.yaml)) — fails a PR that - mixes `terraform/**` with application code, so a Terraform merge never races - a content release for the HCP workspace. +- **Terraform isolation** + ([`.github/workflows/terraform-isolation.yaml`](.github/workflows/terraform-isolation.yaml)) + — fails a PR that mixes `terraform/**` with application code, so a Terraform + merge never races a content release for the HCP workspace. - **Dev content** ([`.github/workflows/deploy.yml`](.github/workflows/deploy.yml)) — `workflow_dispatch` on `dev` only while the Terraform adoption is in progress. Runs `npm run verify`, assumes `githubdeploy-shoc-frontend-new-dev`, diff --git a/scripts/check-terraform-isolation.mjs b/scripts/check-terraform-isolation.mjs index 7b28c149..484b6a50 100644 --- a/scripts/check-terraform-isolation.mjs +++ b/scripts/check-terraform-isolation.mjs @@ -17,7 +17,9 @@ // TERRAFORM_ISOLATION_OVERRIDE=true downgrades a failure to a warning. CI sets // it only when the PR carries the `terraform-isolation-override` label, which // reviewers grant to the rare change that must introduce Terraform variables -// together with the workflow that consumes them. +// together with the workflow that consumes them. The checker has no memory of +// a previous pass: the same mixed diff fails again as soon as the override +// env is unset (label removal). import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; import path from "node:path"; @@ -31,6 +33,13 @@ export function isTerraformPath(file) { return file.startsWith("terraform/"); } +// Markdown under terraform/ does not queue an HCP VCS run (workspace triggers +// are terraform/live/dev/** and terraform/live/modules/**), so it is not a +// Terraform change for the mixed-PR check. +export function isTerraformInfrastructurePath(file) { + return isTerraformPath(file) && !file.endsWith(".md"); +} + export function mayAccompanyTerraform(file) { if (isTerraformPath(file)) return true; if (file.endsWith(".md")) return true; @@ -45,7 +54,7 @@ export function mayAccompanyTerraform(file) { */ export function classifyChangedFiles(files) { const unique = [...new Set(files.map((file) => file.trim()).filter(Boolean))].sort(); - const terraform = unique.filter(isTerraformPath); + const terraform = unique.filter(isTerraformInfrastructurePath); const application = unique.filter((file) => !mayAccompanyTerraform(file)); return { terraform, diff --git a/scripts/check-terraform-isolation.test.mjs b/scripts/check-terraform-isolation.test.mjs index 24c6c87d..45c18fc8 100644 --- a/scripts/check-terraform-isolation.test.mjs +++ b/scripts/check-terraform-isolation.test.mjs @@ -1,5 +1,6 @@ import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; import path from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; @@ -7,6 +8,7 @@ import { fileURLToPath } from "node:url"; import { OVERRIDE_LABEL, classifyChangedFiles, + isTerraformInfrastructurePath, mayAccompanyTerraform, } from "./check-terraform-isolation.mjs"; @@ -71,6 +73,18 @@ test("terraform-only and application-only changes are not mixed", () => { assert.equal(classifyChangedFiles([]).mixed, false); }); +test("terraform documentation does not mix with application or workflow changes", () => { + assert.equal(isTerraformInfrastructurePath("terraform/README.md"), false); + assert.equal(isTerraformInfrastructurePath("terraform/live/dev/main.tf"), true); + assert.equal( + classifyChangedFiles(["terraform/README.md", ".github/workflows/ci.yaml"]).mixed, + false, + ); + const docsOnly = runGate(["terraform/README.md", ".github/workflows/ci.yaml"]); + assert.equal(docsOnly.status, 0, docsOnly.stdout + docsOnly.stderr); + assert.match(docsOnly.stdout, /PASS/); +}); + test("terraform plus application is mixed and lists the offending files", () => { const result = classifyChangedFiles([ "terraform/live/dev/main.tf", @@ -109,7 +123,49 @@ test("CLI override downgrades a mixed change to a warning that names the label", assert.equal(notTrue.status, 1); }); +test("removing the override fails a mixed change that was previously green", () => { + const files = ["terraform/live/dev/main.tf", ".github/workflows/deploy.yml"]; + const previouslyGreen = runGate(files, { + TERRAFORM_ISOLATION_OVERRIDE: "true", + }); + assert.equal(previouslyGreen.status, 0, previouslyGreen.stdout + previouslyGreen.stderr); + assert.match(previouslyGreen.stdout, /WARNING/); + + // CI sets TERRAFORM_ISOLATION_OVERRIDE from contains(...labels), which is + // the string "false" after the label is removed. A stale green check must + // not survive that. + const afterLabelRemoved = runGate(files, { + TERRAFORM_ISOLATION_OVERRIDE: "false", + }); + assert.equal(afterLabelRemoved.status, 1, afterLabelRemoved.stdout + afterLabelRemoved.stderr); + assert.match(afterLabelRemoved.stdout, /FAIL/); + assert.match(afterLabelRemoved.stdout, /deploy\.yml/); +}); + test("CLI refuses to run without a base ref or --stdin", () => { const result = spawnSync(process.execPath, [SCRIPT], { encoding: "utf8" }); assert.notEqual(result.status, 0); }); + +test("isolation workflow re-evaluates on labeled and unlabeled without rerunning Frontend checks", () => { + const workflows = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "..", + ".github/workflows", + ); + const ciYaml = readFileSync(path.join(workflows, "ci.yaml"), "utf8"); + const isolationYaml = readFileSync(path.join(workflows, "terraform-isolation.yaml"), "utf8"); + + for (const eventType of ["opened", "synchronize", "reopened", "labeled", "unlabeled"]) { + assert.match(isolationYaml, new RegExp(`^ {6}- ${eventType}$`, "m"), eventType); + } + + assert.doesNotMatch(ciYaml, /^ {6}- labeled$/m); + assert.doesNotMatch(ciYaml, /^ {6}- unlabeled$/m); + assert.doesNotMatch(ciYaml, /^ {2}terraform-isolation:\n/m); + assert.doesNotMatch(ciYaml, /github\.event\.action != 'labeled'/); + + assert.match(isolationYaml, /^ {2}terraform-isolation:\n/m); + assert.match(isolationYaml, /name: Terraform and application changes are isolated/); + assert.doesNotMatch(isolationYaml, /github\.event\.action != 'labeled'/); +}); diff --git a/terraform/README.md b/terraform/README.md index a20e97cc..e0b2b4ab 100644 --- a/terraform/README.md +++ b/terraform/README.md @@ -218,14 +218,18 @@ the labels. Push-to-`dev` releases return behind the repository variable - **Terraform-only PRs.** A PR that changes `terraform/**` may not change deployable application code. The `terraform-isolation` job in - `.github/workflows/ci.yaml` enforces this; documentation and the + `.github/workflows/terraform-isolation.yaml` enforces this; documentation and the `scripts/*terraform*` tooling are allowed alongside. A reviewer may add the `terraform-isolation-override` label for the rare change that must introduce - Terraform variables together with the workflow that consumes them (PR A and - PR C), then re-run the workflow so the job reads the label. The label is the - approval record. The override is temporary: a follow-up PR after PR C - removes the label path from the checker and workflow so the gate has no - exception. + Terraform variables together with the workflow that consumes them (PR C). + Adding or removing that label re-runs only that workflow against the labels + currently on the PR; Frontend checks does not start a new run. Removing the + label fails a mixed PR that had previously passed with the override, so a + stale green check cannot merge. Markdown under `terraform/` does not count as a Terraform + change for this gate; it does not match the workspace trigger patterns. + The label is the approval record. The override is temporary: + a follow-up PR after PR C removes the label path from the checker and + workflow so the gate has no exception. - **Every Terraform merge produces a VCS run.** A human confirms or discards it before the next content release. Do not leave a pending run on the workspace.