From e7712e6d0f6bdf24e067bd426519eb6aae555a48 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Thu, 1 Oct 2026 20:42:40 -0400 Subject: [PATCH] fix(ci): count deletions and honor the Terraform working directory Deleted paths were excluded from the isolation diff, so a mixed change could pass. The checker now treats working-directory as the Terraform prefix. --- .github/workflows/ci-terraform.yaml | 8 +- scripts/check_app_terraform_isolation.py | 37 ++++- scripts/test_check_app_terraform_isolation.py | 146 ++++++++++++++++++ 3 files changed, 180 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci-terraform.yaml b/.github/workflows/ci-terraform.yaml index 8728926..04cb14c 100644 --- a/.github/workflows/ci-terraform.yaml +++ b/.github/workflows/ci-terraform.yaml @@ -19,7 +19,8 @@ name: CI — Terraform # unchanged. A trailing slash is a directory prefix. Any other line is an # exact file. pull_request classifies the merge-base of the base SHA to HEAD. # merge_group classifies each first-parent commit against its parent, so a -# Terraform-only PR stacked with an app-only PR still passes. +# Terraform-only PR stacked with an app-only PR still passes. The classified +# diff includes deletions. Terraform paths are those under working-directory. on: workflow_call: @@ -85,6 +86,7 @@ jobs: working-directory: ${{ github.workspace }} env: APP_PATHS: ${{ inputs.app-paths }} + TERRAFORM_DIR: ${{ inputs.working-directory }} EVENT_NAME: ${{ github.event_name }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} @@ -101,7 +103,7 @@ jobs: exit 1 fi merge_base="$(git merge-base "${PR_BASE_SHA}" HEAD)" - git diff --name-only --diff-filter=ACMR "${merge_base}" HEAD | classify + git diff --name-only --diff-filter=ACMRD "${merge_base}" HEAD | classify ;; merge_group) if [[ -z "${MERGE_GROUP_BASE_SHA}" ]]; then @@ -110,7 +112,7 @@ jobs: fi while IFS= read -r sha; do [[ -z "${sha}" ]] && continue - git diff --name-only --diff-filter=ACMR "${sha}^" "${sha}" | classify + git diff --name-only --diff-filter=ACMRD "${sha}^" "${sha}" | classify done < <(git rev-list --reverse --first-parent "${MERGE_GROUP_BASE_SHA}..HEAD") ;; *) diff --git a/scripts/check_app_terraform_isolation.py b/scripts/check_app_terraform_isolation.py index d946231..3073498 100644 --- a/scripts/check_app_terraform_isolation.py +++ b/scripts/check_app_terraform_isolation.py @@ -5,6 +5,9 @@ APP_PATHS is newline-separated. A trailing slash is a directory prefix. Any other entry is an exact file. Paths that are not listed are neutral, so workflows, docs, and tests may travel with either side. An empty APP_PATHS skips the check. + +TERRAFORM_DIR is the Terraform working directory (default terraform). A path +is Terraform when it equals that directory or sits under it. """ from __future__ import annotations @@ -28,9 +31,15 @@ def parse_app_rules(raw: str) -> tuple[frozenset[str], frozenset[str]]: return frozenset(prefixes), frozenset(exact) -def is_terraform_path(path: str) -> bool: +def terraform_prefix(terraform_dir: str) -> str: + prefix = terraform_dir.replace("\\", "/").strip().strip("/") + return prefix or "terraform" + + +def is_terraform_path(path: str, terraform_dir: str = "terraform") -> bool: normalized = path.replace("\\", "/") - return normalized == "terraform" or normalized.startswith("terraform/") + prefix = terraform_prefix(terraform_dir) + return normalized == prefix or normalized.startswith(f"{prefix}/") def is_app_path(path: str, prefixes: frozenset[str], exact: frozenset[str]) -> bool: @@ -44,12 +53,16 @@ def is_app_path(path: str, prefixes: frozenset[str], exact: frozenset[str]) -> b def isolation_violation( - paths: list[str], app_paths: str + paths: list[str], + app_paths: str, + terraform_dir: str = "terraform", ) -> tuple[list[str], list[str]] | None: prefixes, exact = parse_app_rules(app_paths) if not prefixes and not exact: return None - terraform_files = sorted({path for path in paths if is_terraform_path(path)}) + terraform_files = sorted( + {path for path in paths if is_terraform_path(path, terraform_dir)} + ) app_files = sorted({path for path in paths if is_app_path(path, prefixes, exact)}) if terraform_files and app_files: return terraform_files, app_files @@ -57,10 +70,12 @@ def isolation_violation( def first_isolation_violation( - file_sets: list[list[str]], app_paths: str + file_sets: list[list[str]], + app_paths: str, + terraform_dir: str = "terraform", ) -> tuple[list[str], list[str]] | None: for paths in file_sets: - violation = isolation_violation(paths, app_paths) + violation = isolation_violation(paths, app_paths, terraform_dir) if violation is not None: return violation return None @@ -77,12 +92,18 @@ def main() -> int: paths = list(args.paths) if not paths and not sys.stdin.isatty(): paths = [line.strip() for line in sys.stdin if line.strip()] - violation = isolation_violation(paths, os.environ.get("APP_PATHS", "")) + terraform_dir = terraform_prefix(os.environ.get("TERRAFORM_DIR", "terraform")) + violation = isolation_violation( + paths, os.environ.get("APP_PATHS", ""), terraform_dir + ) if violation is None: print("PASS: application and Terraform changes are isolated") return 0 terraform_files, app_files = violation - print("FAIL: do not mix deployable application files with terraform/", file=sys.stderr) + print( + f"FAIL: do not mix deployable application files with {terraform_dir}/", + file=sys.stderr, + ) print("terraform:", file=sys.stderr) for path in terraform_files: print(f" {path}", file=sys.stderr) diff --git a/scripts/test_check_app_terraform_isolation.py b/scripts/test_check_app_terraform_isolation.py index 15c122f..a5d1745 100644 --- a/scripts/test_check_app_terraform_isolation.py +++ b/scripts/test_check_app_terraform_isolation.py @@ -3,10 +3,19 @@ from __future__ import annotations +import os +import re +import subprocess +import tempfile import unittest +from pathlib import Path from check_app_terraform_isolation import first_isolation_violation, isolation_violation +WORKFLOW = ( + Path(__file__).resolve().parents[1] / ".github" / "workflows" / "ci-terraform.yaml" +) + APP_PATHS = "src/\npackage.json\npackage-lock.json\n" @@ -76,6 +85,143 @@ class IsolationTests(unittest.TestCase): ) self.assertIsNotNone(violation) + def test_non_default_terraform_dir_mixed_fails(self) -> None: + violation = isolation_violation( + ["infra/main.tf", "src/app.js"], + "src/\n", + terraform_dir="infra", + ) + self.assertIsNotNone(violation) + terraform_files, app_files = violation or ([], []) + self.assertEqual(terraform_files, ["infra/main.tf"]) + self.assertEqual(app_files, ["src/app.js"]) + + def test_default_dir_leaves_other_prefixes_neutral(self) -> None: + self.assertIsNone( + isolation_violation(["infra/main.tf", "src/app.js"], "src/\n") + ) + + def test_terraform_prefix_does_not_match_a_longer_directory(self) -> None: + self.assertIsNone( + isolation_violation( + ["infrastructure/main.tf", "src/app.js"], + "src/\n", + terraform_dir="infra", + ) + ) + + def test_terraform_dir_trailing_slash(self) -> None: + violation = isolation_violation( + ["infra/main.tf", "src/app.js"], + "src/\n", + terraform_dir="infra/", + ) + self.assertIsNotNone(violation) + + def test_blank_terraform_dir_defaults_to_terraform(self) -> None: + violation = isolation_violation( + ["terraform/lambda.tf", "src/app.js"], + "src/\n", + terraform_dir=" ", + ) + self.assertIsNotNone(violation) + + +class WorkflowDiffTests(unittest.TestCase): + def test_classified_diffs_include_deletions(self) -> None: + self.assertEqual(_workflow_diff_filters(), ["ACMRD", "ACMRD"]) + + def test_workflow_passes_working_directory(self) -> None: + self.assertIn( + "TERRAFORM_DIR: ${{ inputs.working-directory }}", + WORKFLOW.read_text(), + ) + + def test_deleted_app_file_is_classified(self) -> None: + diff_filter = _workflow_diff_filters()[0] + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) + base = _commit_base(repo) + (repo / "terraform" / "lambda.tf").write_text("changed\n") + (repo / "src" / "processPaymentCsv.js").unlink() + _git(repo, "add", "-A") + _git(repo, "commit", "-m", "decommission handler") + omitted = _changed_paths(repo, base, "HEAD", "ACMR") + included = _changed_paths(repo, base, "HEAD", diff_filter) + self.assertNotIn("src/processPaymentCsv.js", omitted) + self.assertIn("src/processPaymentCsv.js", included) + self.assertIn("terraform/lambda.tf", included) + self.assertIsNotNone(isolation_violation(included, APP_PATHS)) + + def test_deleted_terraform_file_is_classified(self) -> None: + diff_filter = _workflow_diff_filters()[0] + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) + base = _commit_base(repo) + (repo / "terraform" / "lambda.tf").unlink() + (repo / "src" / "processPaymentCsv.js").write_text("changed\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-m", "remove lambda") + omitted = _changed_paths(repo, base, "HEAD", "ACMR") + included = _changed_paths(repo, base, "HEAD", diff_filter) + self.assertNotIn("terraform/lambda.tf", omitted) + violation = isolation_violation(included, APP_PATHS) + self.assertIsNotNone(violation) + terraform_files, app_files = violation or ([], []) + self.assertEqual(terraform_files, ["terraform/lambda.tf"]) + self.assertEqual(app_files, ["src/processPaymentCsv.js"]) + + +def _workflow_diff_filters() -> list[str]: + return re.findall(r"--diff-filter=([A-Z]+)", WORKFLOW.read_text()) + + +def _git(repo: Path, *args: str) -> str: + env = os.environ.copy() + env["GIT_CONFIG_GLOBAL"] = os.devnull + env["GIT_CONFIG_NOSYSTEM"] = "1" + completed = subprocess.run( + [ + "git", + "-c", + "commit.gpgsign=false", + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + *args, + ], + cwd=repo, + check=True, + capture_output=True, + text=True, + env=env, + ) + return completed.stdout + + +def _commit_base(repo: Path) -> str: + _git(repo, "init", "-b", "main") + (repo / "terraform").mkdir() + (repo / "src").mkdir() + (repo / "terraform" / "lambda.tf").write_text("resource\n") + (repo / "src" / "processPaymentCsv.js").write_text("export {}\n") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "base") + return _git(repo, "rev-parse", "HEAD").strip() + + +def _changed_paths(repo: Path, base: str, head: str, diff_filter: str) -> list[str]: + output = _git( + repo, + "diff", + "--name-only", + f"--diff-filter={diff_filter}", + base, + head, + ) + return [line for line in output.splitlines() if line] + if __name__ == "__main__": unittest.main()