diff --git a/.github/actions/app-terraform-isolation/action.yml b/.github/actions/app-terraform-isolation/action.yml index cad89a5..07db656 100644 --- a/.github/actions/app-terraform-isolation/action.yml +++ b/.github/actions/app-terraform-isolation/action.yml @@ -45,8 +45,15 @@ runs: 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 + # pull_request checkout is the merge commit. Diff that commit + # against its first parent so commits already on the base are + # not classified with this PR. Fall back when HEAD is not a merge. + if git rev-parse --verify --quiet HEAD^2 >/dev/null; then + git diff --name-only --diff-filter=ACMRD HEAD^1 HEAD | classify + else + merge_base="$(git merge-base "${PR_BASE_SHA}" HEAD)" + git diff --name-only --diff-filter=ACMRD "${merge_base}" HEAD | classify + fi ;; merge_group) if [[ -z "${MERGE_GROUP_BASE_SHA}" ]]; then diff --git a/.github/workflows/ci-terraform.yaml b/.github/workflows/ci-terraform.yaml index 29dc9f9..aab5637 100644 --- a/.github/workflows/ci-terraform.yaml +++ b/.github/workflows/ci-terraform.yaml @@ -17,9 +17,10 @@ name: CI — Terraform # # 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 +# exact file. pull_request classifies the merge commit against its first +# parent, not an older event base SHA. 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 diff --git a/scripts/test_check_app_terraform_isolation.py b/scripts/test_check_app_terraform_isolation.py index e367cec..c00b869 100644 --- a/scripts/test_check_app_terraform_isolation.py +++ b/scripts/test_check_app_terraform_isolation.py @@ -134,7 +134,43 @@ class IsolationTests(unittest.TestCase): class WorkflowDiffTests(unittest.TestCase): def test_classified_diffs_include_deletions(self) -> None: - self.assertEqual(_workflow_diff_filters(), ["ACMRD", "ACMRD"]) + self.assertEqual(_workflow_diff_filters(), ["ACMRD", "ACMRD", "ACMRD"]) + + def test_pull_request_diffs_merge_commit_against_first_parent(self) -> None: + action = (ACTION / "action.yml").read_text() + self.assertIn("git rev-parse --verify --quiet HEAD^2", action) + self.assertIn( + "git diff --name-only --diff-filter=ACMRD HEAD^1 HEAD | classify", + action, + ) + self.assertIn('git merge-base "${PR_BASE_SHA}" HEAD', action) + + def test_merge_commit_excludes_commits_already_on_the_base(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) + _commit_base(repo) + old_base = _git(repo, "rev-parse", "HEAD").strip() + (repo / "terraform" / "versions.tf").write_text("aws\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-m", "terraform provider") + (repo / "package.json").write_text("{}\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-m", "npm bump") + _git(repo, "checkout", "-b", "pr", old_base) + workflows = repo / ".github" / "workflows" + workflows.mkdir(parents=True) + (workflows / "ci.yaml").write_text("name: CI\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-m", "pin bump") + _git(repo, "checkout", "main") + _git(repo, "merge", "--no-ff", "pr", "-m", "Merge pr into main") + pr_only = _changed_paths(repo, "HEAD^1", "HEAD", "ACMRD") + stale = _changed_paths(repo, old_base, "HEAD", "ACMRD") + self.assertEqual(pr_only, [".github/workflows/ci.yaml"]) + self.assertIn("package.json", stale) + self.assertIn("terraform/versions.tf", stale) + self.assertIsNone(isolation_violation(pr_only, APP_PATHS)) + self.assertIsNotNone(isolation_violation(stale, APP_PATHS)) def test_workflow_passes_working_directory(self) -> None: self.assertIn(