From 47185fa602dffddb8297db5f3525d7c9bc05d7cd Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Fri, 2 Oct 2026 00:54:43 +0000 Subject: [PATCH] ci(terraform): fail mixed app and Terraform changes (#157) * ci(terraform): fail mixed app and Terraform changes * 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. * fix(ci): load the isolation checker from this workflow's commit The second checkout used the caller's SHA and the caller's token, so a private clone of this repo could not resolve the script. The checker is now a composite action referenced with $/. --- .github/actionlint.yaml | 6 + .../app-terraform-isolation/action.yml | 64 +++++ .../check_app_terraform_isolation.py | 117 +++++++++ .github/workflows/ci-terraform.yaml | 29 +++ .github/workflows/ci.yaml | 4 + scripts/test_check_app_terraform_isolation.py | 239 ++++++++++++++++++ 6 files changed, 459 insertions(+) create mode 100644 .github/actionlint.yaml create mode 100644 .github/actions/app-terraform-isolation/action.yml create mode 100644 .github/actions/app-terraform-isolation/check_app_terraform_isolation.py create mode 100644 scripts/test_check_app_terraform_isolation.py diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml new file mode 100644 index 0000000..ce3828a --- /dev/null +++ b/.github/actionlint.yaml @@ -0,0 +1,6 @@ +# actionlint 1.7.12 rejects `$/`, which GitHub accepts as a self-repository +# action reference (runner 2.336.0+). Drop this ignore when a release parses it. +paths: + .github/workflows/ci-terraform.yaml: + ignore: + - 'specifying action "\$/.github/actions/app-terraform-isolation" in invalid format because ref is missing' diff --git a/.github/actions/app-terraform-isolation/action.yml b/.github/actions/app-terraform-isolation/action.yml new file mode 100644 index 0000000..cad89a5 --- /dev/null +++ b/.github/actions/app-terraform-isolation/action.yml @@ -0,0 +1,64 @@ +name: App and Terraform isolation +description: Fail when a change set mixes Terraform with deployable application files. + +inputs: + app-paths: + description: Newline-separated deployable paths. A trailing slash is a prefix. Any other entry is an exact file. + required: true + terraform-dir: + description: Directory containing Terraform sources. + required: true + default: terraform + event-name: + description: github.event_name from the calling workflow. + required: true + pr-base-sha: + description: pull_request base SHA. Empty outside pull_request. + required: false + default: "" + merge-group-base-sha: + description: merge_group base SHA. Empty outside merge_group. + required: false + default: "" + +runs: + using: composite + steps: + - name: Classify changed paths + shell: bash + working-directory: ${{ github.workspace }} + env: + APP_PATHS: ${{ inputs.app-paths }} + TERRAFORM_DIR: ${{ inputs.terraform-dir }} + EVENT_NAME: ${{ inputs.event-name }} + PR_BASE_SHA: ${{ inputs.pr-base-sha }} + MERGE_GROUP_BASE_SHA: ${{ inputs.merge-group-base-sha }} + CHECKER: ${{ github.action_path }}/check_app_terraform_isolation.py + run: | + set -euo pipefail + classify() { + python3 "${CHECKER}" + } + case "${EVENT_NAME}" in + pull_request) + if [[ -z "${PR_BASE_SHA}" ]]; then + echo "FAIL: pull_request base SHA is empty" >&2 + exit 1 + fi + merge_base="$(git merge-base "${PR_BASE_SHA}" HEAD)" + git diff --name-only --diff-filter=ACMRD "${merge_base}" HEAD | classify + ;; + merge_group) + if [[ -z "${MERGE_GROUP_BASE_SHA}" ]]; then + echo "FAIL: merge_group base SHA is empty" >&2 + exit 1 + fi + while IFS= read -r sha; do + [[ -z "${sha}" ]] && continue + git diff --name-only --diff-filter=ACMRD "${sha}^" "${sha}" | classify + done < <(git rev-list --reverse --first-parent "${MERGE_GROUP_BASE_SHA}..HEAD") + ;; + *) + echo "SKIP: live isolation runs on pull_request and merge_group (event: ${EVENT_NAME})" + ;; + esac diff --git a/.github/actions/app-terraform-isolation/check_app_terraform_isolation.py b/.github/actions/app-terraform-isolation/check_app_terraform_isolation.py new file mode 100644 index 0000000..3073498 --- /dev/null +++ b/.github/actions/app-terraform-isolation/check_app_terraform_isolation.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +"""Fail when a change set mixes Terraform with deployable application files. + +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 + +import argparse +import os +import sys + + +def parse_app_rules(raw: str) -> tuple[frozenset[str], frozenset[str]]: + prefixes: set[str] = set() + exact: set[str] = set() + for line in raw.splitlines(): + item = line.strip().replace("\\", "/") + if not item or item.startswith("#"): + continue + if item.endswith("/"): + prefixes.add(item) + else: + exact.add(item) + return frozenset(prefixes), frozenset(exact) + + +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("\\", "/") + 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: + normalized = path.replace("\\", "/") + if normalized in exact: + return True + for prefix in prefixes: + if normalized.startswith(prefix) or f"{normalized}/" == prefix: + return True + return False + + +def isolation_violation( + 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_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 + return None + + +def first_isolation_violation( + 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, terraform_dir) + if violation is not None: + return violation + return None + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument( + "paths", + nargs="*", + help="Changed paths. Omit and pass newline-separated paths on stdin.", + ) + args = parser.parse_args() + paths = list(args.paths) + if not paths and not sys.stdin.isatty(): + paths = [line.strip() for line in sys.stdin if line.strip()] + 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( + 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) + print("application:", file=sys.stderr) + for path in app_files: + print(f" {path}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/ci-terraform.yaml b/.github/workflows/ci-terraform.yaml index 9935d1c..29dc9f9 100644 --- a/.github/workflows/ci-terraform.yaml +++ b/.github/workflows/ci-terraform.yaml @@ -10,6 +10,20 @@ name: CI — Terraform # uses: Sea-Haven-Industries/.github/.github/workflows/ci-terraform.yaml@ # vX.Y.Z # with: # terraform-version: "1.16.0" +# app-paths: | +# src/ +# package.json +# package-lock.json +# +# app-paths is optional. Empty skips isolation and leaves fmt/init/validate +# 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. The classified +# diff includes deletions. Terraform paths are those under working-directory. +# The checker is a composite action in this repository. `$/` resolves that +# action at this workflow's commit, so callers do not clone this private +# repository with their GITHUB_TOKEN. on: workflow_call: @@ -22,6 +36,10 @@ on: description: "Directory containing Terraform sources" type: string default: "terraform" + app-paths: + description: "Newline-separated deployable paths. A trailing slash is a prefix. Any other entry is an exact file. Empty skips isolation." + type: string + default: "" permissions: contents: read @@ -41,6 +59,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + fetch-depth: 0 - uses: hashicorp/setup-terraform@dfe3c3f87815947d99a8997f908cb6525fc44e9e # v4.0.1 with: @@ -55,3 +74,13 @@ jobs: - name: Terraform validate run: terraform validate + + - name: App and Terraform isolation + if: inputs.app-paths != '' + uses: $/.github/actions/app-terraform-isolation + with: + 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 }} diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 9067c5c..0c7164e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -53,6 +53,10 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Isolation checker tests + run: python3 scripts/test_check_app_terraform_isolation.py + shell: bash + - name: Install actionlint env: ACTIONLINT_VERSION: 1.7.12 diff --git a/scripts/test_check_app_terraform_isolation.py b/scripts/test_check_app_terraform_isolation.py new file mode 100644 index 0000000..e367cec --- /dev/null +++ b/scripts/test_check_app_terraform_isolation.py @@ -0,0 +1,239 @@ +#!/usr/bin/env python3 +"""Tests for check_app_terraform_isolation.""" + +from __future__ import annotations + +import os +import re +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +ACTION = ROOT / ".github" / "actions" / "app-terraform-isolation" +WORKFLOW = ROOT / ".github" / "workflows" / "ci-terraform.yaml" +sys.path.insert(0, str(ACTION)) + +from check_app_terraform_isolation import ( # noqa: E402 + first_isolation_violation, + isolation_violation, +) + +APP_PATHS = "src/\npackage.json\npackage-lock.json\n" + + +class IsolationTests(unittest.TestCase): + def test_terraform_only(self) -> None: + self.assertIsNone( + isolation_violation( + ["terraform/lambda.tf", "terraform/README.md"], + APP_PATHS, + ) + ) + + def test_app_only(self) -> None: + self.assertIsNone( + isolation_violation( + ["src/processPaymentCsv.js", "package.json", "package-lock.json"], + APP_PATHS, + ) + ) + + def test_docs_and_workflows_with_terraform(self) -> None: + self.assertIsNone( + isolation_violation( + [ + "terraform/lambda.tf", + ".github/workflows/deploy.yaml", + "SETUP.md", + "scripts/check_app_terraform_isolation.py", + ], + APP_PATHS, + ) + ) + + def test_mixed_app_and_terraform_fails(self) -> None: + violation = isolation_violation( + ["terraform/lambda.tf", "src/processPaymentCsv.js", "package.json"], + APP_PATHS, + ) + self.assertIsNotNone(violation) + terraform_files, app_files = violation or ([], []) + self.assertEqual(terraform_files, ["terraform/lambda.tf"]) + self.assertEqual(app_files, ["package.json", "src/processPaymentCsv.js"]) + + def test_empty_app_paths_skips(self) -> None: + self.assertIsNone( + isolation_violation( + ["terraform/lambda.tf", "src/processPaymentCsv.js"], + "", + ) + ) + + def test_separate_commits_pass_when_classified_alone(self) -> None: + self.assertIsNone( + first_isolation_violation( + [ + ["terraform/lambda.tf"], + ["src/processPaymentCsv.js"], + ], + APP_PATHS, + ) + ) + + def test_union_of_separate_commits_fails(self) -> None: + violation = isolation_violation( + ["terraform/lambda.tf", "src/processPaymentCsv.js"], + APP_PATHS, + ) + 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_checker_is_this_repos_action_at_the_workflow_commit(self) -> None: + text = WORKFLOW.read_text() + self.assertIn("uses: $/.github/actions/app-terraform-isolation", text) + self.assertNotIn("github.workflow_sha", text) + self.assertNotIn("repository: Sea-Haven-Industries/.github", 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]: + action = (ACTION / "action.yml").read_text() + return re.findall(r"--diff-filter=([A-Z]+)", action) + + +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()