test(e2e): diff the pinned revision and reject globs that match nothing

Three corrections to the affected-only selection, all of the same kind: it could
narrow the matrix while looking healthy.

The changed files now come from git diff --name-only origin/$BASE_REF...HEAD on
whatever was checked out, not from the pull request endpoint. The fork path pins
TARGET_SHA so a push landing after /ok-to-test cannot change what runs against
the approved commit, and reading the live head gave that away. The same
expression serves both paths, since a merge ref and a pinned SHA each yield the
pull request's net changes. Dropping the API call also drops the pagination cap,
the changed_files cross-check and the pull-requests: read grant.
ok-to-test-review.yml now dispatches the base ref, which only the comment path
was sending, so the review path no longer measures against main by accident.

The provider suite's bootstrap was selected by nothing: it sits above every
provider leg's cases/<name>/** globs, so editing it ran core-smoke and skipped
the other eleven. Two check rules stop the class recurring. A glob must match a
tracked file, so a case typo like providers/v1/Azure/** fails the build instead
of quietly retiring its leg, and every suite file must be selected by some
enabled leg. A wrong glob is worse than enabled: false because it leaves no
trace: the leg still passes on any pull request that touches shared machinery.

Also corrects a comment claiming core-smoke runs on fork pull requests. It does
not; a fork runs no leg until /ok-to-test dispatches. Filed as #6794.

Refs: external-secrets/external-secrets#6785
Signed-off-by: Alexander Chernov <alexander@chernov.it>

.github/workflows/e2e-reusable.yml

@@ -99,10 +99,11 @@ env:
   # trusted pull_request path, where the checkout falls back to github.sha (the
   # PR merge ref).
   TARGET_SHA: ${{ github.event.client_payload.slash_command.args.named.sha }}
-  # PR under test, for the affected-only matrix filter. Set on the trusted
-  # pull_request path and in the fork dispatch payload; empty on any other
-  # event, which means run every enabled leg.
-  PR_NUMBER: ${{ github.event.pull_request.number || github.event.client_payload.pull_request.number }}
+  # Base branch the change is measured against, for the affected-only matrix.
+  # The comment path gets it from the dispatched PR object, the review path
+  # from an explicit field in ok-to-test-review.yml. Anything absent or empty
+  # falls back to main, which over-selects rather than under-selects.
+  BASE_REF: ${{ github.event.pull_request.base.ref || github.event.client_payload.pull_request.base.ref || 'main' }}
   # Ephemeral tag: images are only ever loaded into kind, never pushed, so a
   # fixed tag keeps the build and test jobs in sync without passing a version.
   VERSION: "e2e"
@@ -115,7 +116,6 @@ jobs:
     runs-on: ubuntu-latest
     permissions:
       contents: read
-      pull-requests: read # list the PR's changed files, to narrow the matrix
     outputs:
       matrix: ${{ steps.set.outputs.matrix }}
     steps:
@@ -127,33 +127,25 @@ jobs:
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
         with:
           ref: ${{ env.TARGET_SHA || github.sha }}
+          # Full history, blobless, so the affected-only diff below can find a
+          # merge base without paying for file contents it never reads.
+          fetch-depth: 0
+          filter: blob:none
           persist-credentials: false
 
-      # Only narrows the matrix, so every failure path here must end with no
-      # changed.txt, which the next step reads as "run everything". A partial
-      # list is the dangerous case: it looks valid and would silently drop
-      # legs, so publish the file only once its length matches the PR.
-      - name: List the PR's changed files
-        if: env.PR_NUMBER != ''
-        env:
-          GH_TOKEN: ${{ github.token }}
-          REPO: ${{ github.repository }}
+      # Diff the revision that was actually checked out, never the live PR
+      # endpoint: the fork path pins TARGET_SHA so that a push landing after
+      # /ok-to-test cannot change which legs run against the approved commit.
+      # Any failure leaves changed.txt absent, which runs every leg.
+      - name: List the changed files
         run: |
-          if ! gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" \
-               --paginate --jq '.[].filename' > fetched.txt; then
-            echo "::warning::could not list changed files; running every leg"
-            exit 0
-          fi
-          # --paginate stops silently at the API's 3000-file cap, and a
-          # mid-pagination error still leaves the earlier pages on disk.
-          want="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.changed_files')" || want=""
-          got="$(grep -c '' < fetched.txt)"
-          if [ -z "${want}" ] || [ "${want}" != "${got}" ]; then
-            echo "::warning::changed-file list looks incomplete (${got} fetched, PR reports ${want:-unknown}); running every leg"
-            exit 0
+          if git rev-parse --verify --quiet "origin/${BASE_REF}" >/dev/null &&
+             git diff --name-only "origin/${BASE_REF}...HEAD" > fetched.txt; then
+            mv fetched.txt changed.txt
+            echo "changed files: $(grep -c '' < changed.txt)"
+          else
+            echo "::warning::cannot diff against origin/${BASE_REF}; running every leg"
           fi
-          mv fetched.txt changed.txt
-          echo "changed files: ${got}"
 
       - name: Validate and build the e2e matrix
         id: set

.github/workflows/e2e.yml

@@ -28,7 +28,6 @@ jobs:
     permissions:
       id-token: write # for oidc auth with aws/gcp/azure
       contents: read  # for checkout
-      pull-requests: read # prepare-matrix lists changed files to pick legs
     if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && github.actor !='dependabot[bot]'
     uses: ./.github/workflows/e2e-reusable.yml
     secrets:
@@ -136,7 +135,6 @@ jobs:
     permissions:
       id-token: write # for oidc auth with aws/gcp/azure
       contents: read  # for checkout
-      pull-requests: read # prepare-matrix lists changed files to pick legs
     needs: guard-fork
     if: github.event_name == 'repository_dispatch'
     uses: ./.github/workflows/e2e-reusable.yml

.github/workflows/ok-to-test-review.yml

@@ -86,8 +86,9 @@ jobs:
         esac
 
     # Emit the client_payload fields e2e.yml consumes on the comment path:
-    # slash_command.args.named.sha (the target SHA) and pull_request.number
-    # (used by report-fork to comment the result). jq quotes both safely.
+    # slash_command.args.named.sha, pull_request.number (report-fork comments
+    # the result) and pull_request.base.ref (the affected-only matrix diffs
+    # against it). jq quotes all three safely.
     - name: Dispatch ok-to-test-command
       if: steps.cmd.outputs.match == 'true'
       env:
@@ -95,11 +96,13 @@ jobs:
         REPO: ${{ github.repository }}
         SHA: ${{ github.event.review.commit_id }}
         PR_NUMBER: ${{ github.event.pull_request.number }}
+        BASE_REF: ${{ github.event.pull_request.base.ref }}
       run: |
-        jq -cn --arg sha "$SHA" --argjson num "$PR_NUMBER" '{
+        jq -cn --arg sha "$SHA" --argjson num "$PR_NUMBER" \
+               --arg base "$BASE_REF" '{
           event_type: "ok-to-test-command",
           client_payload: {
             slash_command: { args: { named: { sha: $sha } } },
-            pull_request: { number: $num }
+            pull_request: { number: $num, base: { ref: $base } }
           }
         }' | gh api --method POST "repos/${REPO}/dispatches" --input -

e2e/README.md

@@ -89,14 +89,15 @@ Three rules keep this from quietly reducing coverage, all enforced in
   `paths`, so per-area matching alone would skip every provider leg on a core
   change.
 - **Fail open.** No `--changed`, an unreadable file, or an empty list all run
-  the full matrix. A broken diff step must not look like an empty diff. The
-  workflow holds up its end too: it publishes the changed-file list only when
-  the line count matches the PR's own `changed_files`, because `gh api
-  --paginate` stops silently at the API's 3000-file cap and leaves earlier
-  pages on disk if it fails midway. A truncated list is worse than no list,
-  since it narrows the matrix while looking complete.
+  the full matrix. A broken diff step must not look like an empty diff.
 - **Something always runs.** `core-smoke` is `always: true`, so the matrix is
   never empty and the required floor keeps its promise.
+- **The diff comes from the revision under test.** `prepare-matrix` runs
+  `git diff --name-only origin/$BASE_REF...HEAD` on what it checked out, not a
+  query against the live pull request. That matters on the fork path, which
+  pins `TARGET_SHA` so a push landing after `/ok-to-test` cannot change which
+  legs run against the approved commit. It also means the list cannot arrive
+  truncated, the way a paginated API result can.
 
 Matching is `fnmatch.fnmatchcase`, so `providers/v1/aws/**` covers
 `providers/v1/aws/secretsmanager/client.go` but not `providers/v1/awsx/`. Case
@@ -220,8 +221,19 @@ make -C e2e test.run TEST_SUITES=provider GINKGO_LABELS="vault && !managed" \
    and wire that group's env vars in `e2e-reusable.yml`.
 4. Set `enabled: true` when you want CI to run it.
 
-`matrix.py check` (run in `prepare-matrix`) enforces steps 1-3: it fails the
-build if a provider is compiled into the suite but not covered by an area, if
-`needs_secrets` disagrees with `secret_groups`, if an area names a secret group
-that the workflow does not wire, or if an enabled area declares no `paths`,
-which would leave it sitting out nearly every PR.
+`matrix.py check` (run in `prepare-matrix`) enforces steps 1-3, and fails the
+build on any of:
+
+- a provider compiled into the suite but not covered by an area;
+- `needs_secrets` disagreeing with `secret_groups`;
+- an area naming a secret group the workflow does not wire;
+- an enabled area declaring no `paths`, which would leave it sitting out
+  nearly every PR;
+- a glob that matches no tracked file, so a typo cannot quietly stop selecting
+  its leg;
+- a suite's own file that no enabled leg selects, which is how the provider
+  suite's bootstrap slipped through once.
+
+The last two exist because a wrong glob is invisible in a way that
+`enabled: false` never was: the leg keeps passing on every PR that happens to
+touch shared machinery, so nothing looks broken.

e2e/matrix.py

@@ -77,6 +77,16 @@ def group_to_vars() -> dict[str, list[str]]:
     return mapping
 
 
+def tracked_files() -> list[str]:
+    """Repo-relative tracked paths. The glob rules in check need to know that a
+    pattern corresponds to something real."""
+    out = subprocess.run(
+        ["git", "-C", str(HERE.parent), "ls-files"],
+        check=True, capture_output=True, text=True,
+    ).stdout
+    return out.splitlines()
+
+
 def cmd_check(matrix: dict) -> int:
     areas = matrix["areas"]
     errors: list[str] = []
@@ -119,6 +129,33 @@ def cmd_check(matrix: dict) -> int:
                 "affected-only selection would almost never run it"
             )
 
+    shared = matrix.get("full_matrix_paths") or []
+    live = [a for a in areas if a.get("enabled")]
+    files = tracked_files()
+
+    # 5. Every glob must match something. A typo like providers/v1/Azure/**
+    # stops selecting its leg while check and selftest stay green, and unlike
+    # enabled: false it leaves no trace on later pull requests.
+    labelled = [("full_matrix_paths", g) for g in shared]
+    labelled += [(f"area {a['name']!r}", g) for a in live
+                 for g in (a.get("paths") or [])]
+    for where, glob in labelled:
+        if not any(fnmatchcase(f, glob) for f in files):
+            errors.append(f"{where}: glob {glob!r} matches no tracked file")
+
+    # 6. Every suite's own files must reach some leg. They sit one level above
+    # the per-case globs, so the provider suite's bootstrap was selected by
+    # nothing until it was listed explicitly.
+    selectable = shared + [g for a in live for g in (a.get("paths") or [])]
+    for f in files:
+        parts = f.split("/")
+        if len(parts) == 4 and parts[:2] == ["e2e", "suites"]:
+            if not any(fnmatchcase(f, g) for g in selectable):
+                errors.append(
+                    f"suite file {f} is selected by no enabled leg, so a "
+                    "change to it would skip the legs that run it"
+                )
+
     if errors:
         print("ERROR: matrix.yaml is inconsistent:", file=sys.stderr)
         for e in errors:
@@ -251,6 +288,9 @@ SELFTEST_CASES: list[tuple[list[str], set[str] | None]] = [
     # Every leg runs the same image and cluster, so these are shared too. They
     # were missed once; a change here skipping the vault leg is the exact
     # coverage loss affected-only selection must never cause.
+    # Every provider leg compiles this bootstrap, and it sits above their
+    # cases/<name>/** globs, so nothing else would select it.
+    (["e2e/suites/provider/suite_test.go"], ALL),
     (["e2e/Dockerfile"], ALL),
     (["e2e/entrypoint.sh"], ALL),
     (["e2e/k8s/vault.values.yaml"], ALL),

e2e/matrix.yaml

@@ -74,10 +74,15 @@ full_matrix_paths:
   - "e2e/k8s/**"
   - "e2e/suites/provider/cases/common/**"
   - "e2e/suites/provider/cases/import.go"
+  # The provider suite's bootstrap sits one level above every provider leg's
+  # cases/<name>/** globs, so nothing else selects it. check rule 6 keeps any
+  # future sibling from going uncovered the same way.
+  - "e2e/suites/provider/suite_test.go"
 
 areas:
-  # In-cluster only, no external credentials. The required floor: this leg runs
-  # on every PR (including forks) and is the stable branch-protection gate.
+  # In-cluster only, no external credentials. The required floor: never
+  # filtered out, so it runs whatever a pull request changed. Not "including
+  # forks": a fork PR runs no leg at all until /ok-to-test dispatches.
   - name: core-smoke
     suite: provider
     labels: "(fake || kubernetes || template) && !managed"