Просмотр исходного кода

test(e2e): run only the legs a pull request can affect

Each area already carried a paths list seeded for this. Consume it: prepare-matrix lists the PR's
changed files and matrix.py keeps the legs those files can reach, so a one-provider change stops
paying for the whole fan-out and a flaky vendor leg stops failing PRs that go nowhere near it.

Narrowing coverage by accident is the risk, so the rules that prevent it live in matrix.py rather
than in workflow prose: shared machinery selects every leg, core-smoke is never filtered out, and
anything unusable about the changed-file list means run everything. The workflow publishes that
list only when its length matches the PR, since gh api --paginate stops silently at the 3000-file
cap and a partial list narrows the matrix while looking complete.

Fixes: external-secrets/external-secrets#6785
Signed-off-by: Alexander Chernov <alexander@chernov.it>
Alexander Chernov 1 месяц назад
Родитель
Сommit
89d8cab894
4 измененных файлов с 331 добавлено и 22 удалено
  1. 37 1
      .github/workflows/e2e-reusable.yml
  2. 49 4
      e2e/README.md
  3. 192 9
      e2e/matrix.py
  4. 53 8
      e2e/matrix.yaml

+ 37 - 1
.github/workflows/e2e-reusable.yml

@@ -99,6 +99,10 @@ 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 }}
   # 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"
@@ -111,6 +115,7 @@ 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:
@@ -124,6 +129,32 @@ jobs:
           ref: ${{ env.TARGET_SHA || github.sha }}
           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 }}
+        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
+          fi
+          mv fetched.txt changed.txt
+          echo "changed files: ${got}"
+
       - name: Validate and build the e2e matrix
         id: set
         # This job has no secrets in scope. matrix.py reads only matrix.yaml and
@@ -131,8 +162,13 @@ jobs:
         # without ever touching a secret value.
         run: |
           ./e2e/matrix.py check
+          ./e2e/matrix.py selftest
           ./e2e/matrix.py plan
-          matrix="$(./e2e/matrix.py json)"
+          if [ -s changed.txt ]; then
+            matrix="$(./e2e/matrix.py json --changed changed.txt)"
+          else
+            matrix="$(./e2e/matrix.py json)"
+          fi
           echo "matrix=${matrix}" >> "$GITHUB_OUTPUT"
 
   build:

+ 49 - 4
e2e/README.md

@@ -67,10 +67,54 @@ Each `area` is one leg:
   providers: [aws]                # for the coverage check only
   secret_groups: [aws]            # which credential groups this leg receives
   needs_secrets: true             # mirror of "secret_groups is non-empty"
-  paths:                          # phase 2 seed (affected-only), unused today
+  paths:                          # globs that select this leg on a PR
     - "providers/v1/aws/**"
     - "e2e/suites/provider/cases/aws/**"
-  enabled: true                   # whether the phase 1 matrix runs it now
+  enabled: true                   # whether CI runs it at all
+```
+
+## Affected-only selection
+
+On a pull request, a leg runs only when the diff can affect it. `prepare-matrix`
+lists the PR's changed files and passes them to `matrix.py json --changed`,
+which keeps an enabled area when either its `paths` globs match or it is marked
+`always: true`. Any other event runs the full matrix.
+
+Three rules keep this from quietly reducing coverage, all enforced in
+`matrix.py` rather than in workflow YAML:
+
+- **Shared machinery runs everything.** A change matching the top-level
+  `full_matrix_paths` selects every enabled leg. This is load-bearing, not
+  belt-and-braces: `apis/`, `pkg/` and `runtime/` appear in only four areas'
+  `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.
+- **Something always runs.** `core-smoke` is `always: true`, so the matrix is
+  never empty and the required floor keeps its promise.
+
+Matching is `fnmatch.fnmatchcase`, so `providers/v1/aws/**` covers
+`providers/v1/aws/secretsmanager/client.go` but not `providers/v1/awsx/`. Case
+is significant, so a laptop and a Linux runner agree.
+
+Careful when editing either list: `fnmatch`'s `*` crosses `/`, unlike a shell
+glob. `e2e/*` therefore matches `e2e/suites/provider/cases/aws/x.go` as well as
+`e2e/Dockerfile`, which would quietly make every change run the full matrix.
+That is why the shared `e2e` entries are listed file by file.
+
+`matrix.py selftest` checks the resolver against a table of changed-file sets
+and their expected legs, and runs in `prepare-matrix` beside `check`. Extend it
+when you change the selection rules.
+
+To see what a given diff would select:
+
+```bash
+git diff --name-only origin/main... | ./e2e/matrix.py json --changed -
 ```
 
 Notes:
@@ -178,5 +222,6 @@ make -C e2e test.run TEST_SUITES=provider GINKGO_LABELS="vault && !managed" \
 
 `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`, or if an area names a secret
-group that the workflow does not wire.
+`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.

+ 192 - 9
e2e/matrix.py

@@ -6,8 +6,10 @@ Subcommands:
           the suite (suites/provider/cases/import.go) is not covered by any
           area, needs_secrets disagrees with secret_groups, or an area names a
           secret group that the reusable workflow does not wire up.
-  json    Print the GitHub Actions matrix (enabled areas only) as compact JSON
-          for the workflow's strategy.matrix.
+  json    Print the GitHub Actions matrix as compact JSON for the workflow's
+          strategy.matrix. With --changed <file|->, keep only the legs that
+          change can affect. See "Affected-only selection" in e2e/README.md.
+  selftest Check the affected-only resolver against a table of known cases.
   plan    Print, per enabled leg, exactly which credential env vars it will
           receive. Derived from each area's secret_groups and the group -> var
           mapping parsed out of e2e-reusable.yml. This reads NO secret values
@@ -19,12 +21,18 @@ matter. YAML is read with PyYAML when present, else via yq (mikefarah), so no
 new runtime dependency is required in CI.
 """
 
+import io
 import json
 import re
 import subprocess
 import sys
+from contextlib import redirect_stderr
+from fnmatch import fnmatchcase
 from pathlib import Path
 
+USAGE = "usage: matrix.py [check|json [--changed <file|->]|plan|selftest]"
+Match = tuple[str, str] | None
+
 HERE = Path(__file__).resolve().parent
 MATRIX = HERE / "matrix.yaml"
 IMPORT = HERE / "suites/provider/cases/import.go"
@@ -102,6 +110,15 @@ def cmd_check(matrix: dict) -> int:
                     f"in {WORKFLOW.name} (no env var gates on it)"
                 )
 
+    # 4. Selection depends on paths, so an enabled area without them would run
+    # only when full_matrix_paths hits and silently sit out every other PR.
+    for a in areas:
+        if a.get("enabled") and not a.get("paths"):
+            errors.append(
+                f"area {a['name']!r}: enabled but declares no paths, so "
+                "affected-only selection would almost never run it"
+            )
+
     if errors:
         print("ERROR: matrix.yaml is inconsistent:", file=sys.stderr)
         for e in errors:
@@ -116,7 +133,71 @@ def cmd_check(matrix: dict) -> int:
     return 0
 
 
-def cmd_json(matrix: dict) -> int:
+def parse_changed(text: str, source: str) -> list[str] | None:
+    """Non-blank lines of text, or None for "nothing usable, run everything".
+    An empty list is indistinguishable from a diff step that produced
+    nothing, so it must not narrow the matrix."""
+    paths = [line.strip() for line in text.splitlines() if line.strip()]
+    if not paths:
+        print(f"WARNING: no changed paths in {source}; running the full matrix",
+              file=sys.stderr)
+        return None
+    return paths
+
+
+def read_changed(source: str) -> list[str] | None:
+    """Changed paths, one per line, from a file or stdin ("-"). None means
+    "run everything": an unreadable file is a broken diff step, not an empty
+    diff, so it may not narrow the matrix either."""
+    try:
+        text = sys.stdin.read() if source == "-" else Path(source).read_text()
+    except OSError as err:
+        print(f"WARNING: cannot read changed paths from {source!r} ({err}); "
+              "running the full matrix", file=sys.stderr)
+        return None
+    return parse_changed(text, repr(source))
+
+
+def first_match(patterns: list[str], paths: list[str]) -> Match:
+    """First (pattern, path) pair that matches, else None. fnmatchcase, not
+    fnmatch: the latter normalises case per platform, so a laptop and a Linux
+    runner would disagree."""
+    for pattern in patterns:
+        for path in paths:
+            if fnmatchcase(path, pattern):
+                return pattern, path
+    return None
+
+
+def select_areas(matrix: dict, changed: list[str] | None) -> list[dict]:
+    """Enabled areas a change can affect; changed=None runs all of them. An
+    area is kept when it is always-on or one of its paths globs matches, and
+    full_matrix_paths keeps every area. See matrix.yaml for why that is not
+    merely defensive."""
+    enabled = [a for a in matrix["areas"] if a.get("enabled")]
+    if changed is None:
+        return enabled
+
+    if hit := first_match(matrix.get("full_matrix_paths") or [], changed):
+        print(f"full matrix: {hit[1]} matches full_matrix_paths {hit[0]!r}",
+              file=sys.stderr)
+        return enabled
+
+    selected, dropped = [], []
+    for a in enabled:
+        if a.get("always") or first_match(a.get("paths") or [], changed):
+            selected.append(a)
+        else:
+            dropped.append(a["name"])
+    if dropped:
+        print(f"affected-only: {len(selected)} of {len(enabled)} leg(s) "
+              f"selected from {len(changed)} changed file(s); skipping "
+              + ", ".join(dropped), file=sys.stderr)
+    return selected
+
+
+def cmd_json(matrix: dict, changed_from: str | None = None) -> int:
+    changed = read_changed(changed_from) if changed_from else None
     include = [
         {
             "name": a["name"],
@@ -124,8 +205,7 @@ def cmd_json(matrix: dict) -> int:
             "labels": a["labels"],
             "secret_groups": a.get("secret_groups") or [],
         }
-        for a in matrix["areas"]
-        if a.get("enabled")
+        for a in select_areas(matrix, changed)
     ]
     print(json.dumps({"include": include}, separators=(",", ":")))
     return 0
@@ -146,13 +226,116 @@ def cmd_plan(matrix: dict) -> int:
     return 0
 
 
+# (changed paths, expected leg names or ALL) for the resolver. Guards the two
+# rules whose silent failure costs coverage: fail open, and a shared-machinery
+# change running every leg. See cmd_selftest.
+ALL = None  # in the table below: expect every enabled leg
+SELFTEST_CASES: list[tuple[list[str], set[str] | None]] = [
+    # A provider change runs that provider plus the always-on floor.
+    (["providers/v1/vault/client.go"], {"core-smoke", "vault"}),
+    (["e2e/suites/provider/cases/aws/secretsmanager.go"],
+     {"core-smoke", "aws"}),
+    # grafana.go selects the generator leg too: both legs run the same suite
+    # binary from the same Go package, so a change here can break its compile.
+    (["e2e/suites/generator/grafana.go"],
+     {"core-smoke", "generator", "grafana"}),
+    # Two providers at once select both.
+    (["providers/v1/vault/x.go", "providers/v1/gcp/y.go"],
+     {"core-smoke", "vault", "gcp"}),
+    # Shared machinery runs everything, even though most areas do not list it.
+    (["runtime/reconciler.go"], ALL),
+    (["apis/externalsecrets/v1/types.go"], ALL),
+    (["e2e/framework/util.go"], ALL),
+    (["e2e/matrix.yaml"], ALL),
+    ([".github/workflows/e2e-reusable.yml"], ALL),
+    # 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.
+    (["e2e/Dockerfile"], ALL),
+    (["e2e/entrypoint.sh"], ALL),
+    (["e2e/k8s/vault.values.yaml"], ALL),
+    (["e2e/kind.yaml"], ALL),
+    # Unrelated changes still run the floor, never an empty matrix. e2e docs
+    # are deliberately not shared machinery.
+    (["docs/introduction/faq.md"], {"core-smoke"}),
+    (["e2e/README.md"], {"core-smoke"}),
+    # A near miss must not match: awsx is not aws.
+    (["providers/v1/awsx/client.go"], {"core-smoke"}),
+    # Fail open: nothing to go on means run everything.
+    ([], ALL),
+]
+
+
+def cmd_selftest(matrix: dict) -> int:
+    """Exercise select_areas against SELFTEST_CASES. Runs in prepare-matrix
+    beside check, so a regression in the resolver fails the build rather than
+    quietly shrinking the fan-out."""
+    every = {a["name"] for a in matrix["areas"] if a.get("enabled")}
+    failures = 0
+
+    def fail(what: str, detail: str) -> None:
+        nonlocal failures
+        failures += 1
+        print(f"FAIL {what}\n  {detail}", file=sys.stderr)
+
+    for changed, expected in SELFTEST_CASES:
+        want = every if expected is None else expected
+        # select_areas narrates its decision on stderr; capture it so a real
+        # failure is not buried, and so the narration can be asserted on.
+        log = io.StringIO()
+        with redirect_stderr(log):
+            got = {a["name"] for a in select_areas(matrix, changed or None)}
+        if got != want:
+            fail(f"changed={changed}",
+                 f"want {sorted(want)}\n  got  {sorted(got)}")
+        # A wrongly skipped leg is diagnosed from this log, so it has to name
+        # the legs it dropped, or the reason a change ran everything.
+        if changed and got != every and "skipping" not in log.getvalue():
+            fail(f"changed={changed}", "narrowed the matrix, logged no why")
+        if changed and got == every and "full matrix" not in log.getvalue():
+            fail(f"changed={changed}", "ran every leg without logging why")
+
+    # These two own the remaining fail-open rules and select_areas never
+    # reaches them, so exercise them directly rather than trusting them.
+    with redirect_stderr(io.StringIO()):
+        cases = [
+            ("unreadable file", read_changed(str(HERE / "no-such-file")), None),
+            ("blank text", parse_changed("  \n\t\n", "<test>"), None),
+            ("two paths", parse_changed("a/b.go\n c/d.go \n", "<test>"),
+             ["a/b.go", "c/d.go"]),
+        ]
+    for what, got_paths, want_paths in cases:
+        if got_paths != want_paths:
+            fail(f"read_changed/parse_changed on {what}",
+                 f"want {want_paths}, got {got_paths}")
+
+    total = len(SELFTEST_CASES) + len(cases)
+    if failures:
+        print(f"ERROR: {failures} of {total} selftest assertion(s) failed",
+              file=sys.stderr)
+        return 1
+    print(f"matrix.py selftest ok: {total} cases")
+    return 0
+
+
 def main() -> int:
-    cmd = sys.argv[1] if len(sys.argv) > 1 else "check"
-    if cmd not in ("check", "json", "plan"):
-        print(f"usage: {sys.argv[0]} [check|json|plan]", file=sys.stderr)
+    argv = sys.argv[1:]
+    cmd = argv[0] if argv else "check"
+    others = {"check": cmd_check, "plan": cmd_plan, "selftest": cmd_selftest}
+    changed_from = None
+    if cmd == "json":
+        if len(argv) > 1:
+            if argv[1] != "--changed" or len(argv) != 3:
+                print(USAGE, file=sys.stderr)
+                return 2
+            changed_from = argv[2]
+    elif len(argv) > 1 or cmd not in others:
+        print(USAGE, file=sys.stderr)
         return 2
     matrix = load_yaml(MATRIX)
-    return {"check": cmd_check, "json": cmd_json, "plan": cmd_plan}[cmd](matrix)
+    if cmd == "json":
+        return cmd_json(matrix, changed_from)
+    return others[cmd](matrix)
 
 
 if __name__ == "__main__":

+ 53 - 8
e2e/matrix.yaml

@@ -1,7 +1,9 @@
 # Source of truth for the non-managed e2e fan-out (e2e.yml). Each area is one CI
 # leg: a suite binary run under a Ginkgo label filter. The build job compiles
-# the images once; every enabled area runs as its own matrix leg on its own kind
-# cluster, so a flaky addon in one provider cannot fail the others.
+# the images once; each selected area runs as its own matrix leg on its own kind
+# cluster, so a flaky addon in one provider cannot fail the others. On a pull
+# request the selection is the legs the diff can affect (see paths below); every
+# other event runs all enabled areas.
 #
 # Fields:
 #   name          leg id, shown as "test (<name>)" in the checks list.
@@ -21,16 +23,58 @@
 #                 names map to secret sets in e2e-reusable.yml.
 #   needs_secrets convenience mirror of "secret_groups is non-empty". Documents
 #                 intent; not read by CI.
-#   paths         globs whose change should trigger this leg. Seed for the
-#                 phase 2 affected-only resolver; ignored by the static phase 1
-#                 matrix, which runs every enabled area.
-#   enabled       whether phase 1 CI runs this leg now. Disabled areas still
-#                 count for coverage and document the target matrix; flip to true
-#                 to expand the fan-out.
+#   paths         globs whose change triggers this leg. Read by the resolver
+#                 (matrix.py json --changed) on pull requests; every enabled
+#                 area runs on other events. Required on enabled areas, or the
+#                 leg would sit out nearly every PR.
+#   always        opt this leg out of affected-only filtering, so it runs on
+#                 every PR whatever changed. Set on the required floor only:
+#                 something must always run, or the matrix could come out empty.
+#   enabled       whether CI runs this leg at all. Disabled areas still count
+#                 for coverage and document the target matrix; flip to true to
+#                 expand the fan-out.
 #
 # All legs are enabled. Set enabled: false on an area to skip it (e.g. while a
 # provider's e2e is being fixed) without deleting its definition.
 
+# Shared machinery no single leg owns: a change here runs every enabled leg.
+# Not belt-and-braces: apis/, pkg/ and runtime/ appear in only four areas'
+# paths, so per-area matching alone would skip every provider leg on a core
+# change, losing coverage exactly when it matters most.
+#
+# The e2e entries are listed file by file on purpose. Globs are fnmatch, where
+# * crosses /, so "e2e/*" would also match e2e/suites/... and make every change
+# run everything. e2e/README.md is left out so a docs edit stays cheap.
+full_matrix_paths:
+  - "apis/**"
+  - "pkg/**"
+  - "runtime/**"
+  - "cmd/**"
+  - "deploy/**"
+  - "hack/**"
+  - "go.mod"
+  - "go.sum"
+  - "Makefile"
+  - "Dockerfile*"
+  - ".github/workflows/**"
+  # Every leg runs the same image and the same kind cluster, so all of this is
+  # shared: the Dockerfile ADDs entrypoint.sh, k8s/ and all four suite
+  # binaries, and the Makefile creates the cluster from kind.yaml.
+  - "e2e/Dockerfile"
+  - "e2e/entrypoint.sh"
+  - "e2e/go.mod"
+  - "e2e/go.sum"
+  - "e2e/kind.yaml"
+  - "e2e/Makefile"
+  - "e2e/matrix.py"
+  - "e2e/matrix.yaml"
+  - "e2e/run.sh"
+  - "e2e/tools.go"
+  - "e2e/framework/**"
+  - "e2e/k8s/**"
+  - "e2e/suites/provider/cases/common/**"
+  - "e2e/suites/provider/cases/import.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.
@@ -51,6 +95,7 @@ areas:
       - "e2e/suites/provider/cases/kubernetes/**"
       - "e2e/suites/provider/cases/template/**"
       - "e2e/suites/provider/cases/common/**"
+    always: true
     enabled: true
 
   # The CRD provider reads arbitrary custom resources from the local cluster