diff --git a/.github/workflows/ci-conformance.yml b/.github/workflows/ci-conformance.yml index fdd8950..b0df89b 100644 --- a/.github/workflows/ci-conformance.yml +++ b/.github/workflows/ci-conformance.yml @@ -4,9 +4,13 @@ name: CI conformance # requires this workflow on every repository whose ci-tier property is # merge-queue. It runs from this repository, on the target repository's pull # requests and merge groups, and checks the target's workflows against CI.md. +# +# A ruleset runs a required workflow on the default activity types only +# (opened, synchronize, reopened, and the merge group's checks_requested) and +# ignores `types` and other filters. So the job runs on drafts too: a draft +# marked ready gets no new event, and the check is cheap. on: pull_request: - types: [opened, synchronize, reopened, ready_for_review, stacked] merge_group: concurrency: @@ -19,7 +23,7 @@ permissions: jobs: ci-conformance: # This repository has no ci-gate of its own; self-test.yml covers it. - if: github.event.pull_request.draft != true && github.repository != 'cocoindex-io/.github' + if: github.repository != 'cocoindex-io/.github' runs-on: ubuntu-latest steps: - uses: actions/checkout@v5 diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..c18dd8d --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +__pycache__/ diff --git a/CI.md b/CI.md index ca7d447..2966c00 100644 --- a/CI.md +++ b/CI.md @@ -49,7 +49,11 @@ workflow has all of them. in its `if`, or needs a job that has it. 4. **A `changes` job** runs `cocoindex-io/.github/actions/ci-mode@v1` (it needs `checks: read`) and the path filter. Expensive jobs run when - `full == 'true'` and the filter output is not `'false'`. + `full == 'true'` and the filter output is not `'false'`. The filter is + skipped for a stack layer, and so is any other diff against the pull + request's base branch: a stack layer's base is the layer below it, but + the top layer's run stands in for the whole stack. Diff a stack layer + against `github.event.pull_request.stack.base.ref`. 5. **A `ci-gate` job** with `if: always()` that needs every other job. It is the one check to require. A skipped job reports success, so a required check that can be skipped by a failure would let the failure merge. @@ -57,7 +61,10 @@ workflow has all of them. Every other workflow that runs on pull requests has parts 2 and 3, the `ready_for_review` type, and the `stacked` type when it filters on branches. Every workflow that reports a required check triggers on `merge_group`, or -the queue waits for that check until it times out. +the queue waits for that check until it times out. Require `ci-gate`, plus +checks from jobs that run in every tier, such as `fast-check`. A matrix job +that the tier skips reports one check under its unexpanded name, so a +required `name (value)` check from it never arrives. ## The merge queue @@ -121,6 +128,10 @@ repository's own rulesets. the Change Management Policy asks that the change be announced. - Check a repository locally: `uv run --with pyyaml scripts/check_ci_conformance.py PATH --repo OWNER/NAME`. +- A ruleset runs a required workflow only on `opened`, `synchronize` and + `reopened` pull request events and on merge groups, and ignores the + workflow's `types` and filters. `ci-conformance` therefore runs on drafts + too. ## Changing this kit diff --git a/scripts/__pycache__/apply_rulesets.cpython-314.pyc b/scripts/__pycache__/apply_rulesets.cpython-314.pyc deleted file mode 100644 index 6421b77..0000000 Binary files a/scripts/__pycache__/apply_rulesets.cpython-314.pyc and /dev/null differ diff --git a/scripts/__pycache__/check_ci_conformance.cpython-314.pyc b/scripts/__pycache__/check_ci_conformance.cpython-314.pyc deleted file mode 100644 index f31f7bf..0000000 Binary files a/scripts/__pycache__/check_ci_conformance.cpython-314.pyc and /dev/null differ diff --git a/scripts/__pycache__/test_apply_rulesets.cpython-314-pytest-9.1.1.pyc b/scripts/__pycache__/test_apply_rulesets.cpython-314-pytest-9.1.1.pyc deleted file mode 100644 index 36ff4d3..0000000 Binary files a/scripts/__pycache__/test_apply_rulesets.cpython-314-pytest-9.1.1.pyc and /dev/null differ diff --git a/scripts/__pycache__/test_check_ci_conformance.cpython-314-pytest-9.1.1.pyc b/scripts/__pycache__/test_check_ci_conformance.cpython-314-pytest-9.1.1.pyc deleted file mode 100644 index afea302..0000000 Binary files a/scripts/__pycache__/test_check_ci_conformance.cpython-314-pytest-9.1.1.pyc and /dev/null differ diff --git a/scripts/apply_rulesets.py b/scripts/apply_rulesets.py index e80b102..c87812c 100644 --- a/scripts/apply_rulesets.py +++ b/scripts/apply_rulesets.py @@ -69,7 +69,10 @@ def sync_property(org: str, check: bool) -> bool: def sync_ruleset(org: str, path: pathlib.Path, check: bool) -> bool: want = json.loads(path.read_text()) - existing = {r["name"]: r["id"] for r in gh(f"orgs/{org}/rulesets", "--paginate")} + # `--slurp` wraps the pages in one array; `--paginate` alone prints one + # JSON array per page, which is not one JSON document. + pages = gh(f"orgs/{org}/rulesets", "--paginate", "--slurp") + existing = {r["name"]: r["id"] for page in pages for r in page} if want["name"] in existing: have = gh(f"orgs/{org}/rulesets/{existing[want['name']]}") found = differences(want, have) diff --git a/scripts/check_ci_conformance.py b/scripts/check_ci_conformance.py index db5ce37..dac65e7 100644 --- a/scripts/check_ci_conformance.py +++ b/scripts/check_ci_conformance.py @@ -25,6 +25,9 @@ CI_MODE_ACTION = "cocoindex-io/.github/actions/ci-mode@" GATE_JOB = "ci-gate" GITHUB_ACTIONS_APP_ID = 15368 +# Status functions in a job's `if` replace the implicit success() check, so +# the job runs even when a job it needs was skipped. +STATUS_FUNCTIONS = ("always()", "cancelled()", "failure()") class Finding(NamedTuple): @@ -71,14 +74,19 @@ def fail(message: str) -> None: if (has_gate or branch_filtered) and "stacked" not in types: fail("pull_request.types must list stacked: a stack layer whose opened event arrives before its " "stack exists misses the branch filter and runs no CI.") - group = _text((doc.get("concurrency") or {}).get("group")) - cancel = (doc.get("concurrency") or {}).get("cancel-in-progress") + concurrency = doc.get("concurrency") or {} + if isinstance(concurrency, str): # `concurrency: ` is the short form + concurrency = {"group": concurrency} + group = _text(concurrency.get("group")) + cancel = concurrency.get("cancel-in-progress") if "pull_request.number" not in group or cancel in (None, False): fail("A workflow-level concurrency group keyed on github.event.pull_request.number, " "with cancel-in-progress, must cancel a pull request's superseded runs.") for job_id, job in jobs.items(): condition = _text(job.get("if")) - inherits_skip = bool(job.get("needs")) and "always()" not in condition + # A job that needs a skipped job is skipped too, unless a status + # function in its `if` overrides the implicit success() check. + inherits_skip = bool(job.get("needs")) and not any(f in condition for f in STATUS_FUNCTIONS) if "pull_request.draft" not in condition and not inherits_skip: fail(f"Job {job_id} must skip drafts: add `github.event.pull_request.draft != true` to its if.") @@ -108,19 +116,56 @@ def fail(message: str) -> None: return findings -def check_name_prefixes(doc: dict[Any, Any]) -> list[str]: - """Return the check-name prefixes the workflow's jobs report under.""" - prefixes = [] - for job_id, job in (doc.get("jobs") or {}).items(): - name = _text(job.get("name")) or job_id - prefixes.append(name.split("${{")[0].strip()) - return prefixes +def _needs(job: dict[str, Any]) -> list[str]: + needs = job.get("needs") or [] + return [needs] if isinstance(needs, str) else list(needs) -def check_required_contexts(contexts: list[str], merge_group_docs: list[dict[Any, Any]]) -> list[str]: - """Return the required contexts that no merge_group workflow reports.""" - prefixes = [p for doc in merge_group_docs for p in check_name_prefixes(doc)] - return [c for c in contexts if not any(p and c.startswith(p) for p in prefixes)] +def _tier_skipped_jobs(jobs: dict[str, Any]) -> set[str]: + """Jobs a fast-tier run skips: those whose `if` reads the tier, and those that need one.""" + skipped = {job_id for job_id, job in jobs.items() if "outputs.full" in _text(job.get("if"))} + grew = True + while grew: + grew = False + for job_id, job in jobs.items(): + if (job_id not in skipped and any(n in skipped for n in _needs(job)) + and not any(f in _text(job.get("if")) for f in STATUS_FUNCTIONS)): + skipped.add(job_id) + grew = True + return skipped + + +def _reports(job_id: str, job: dict[str, Any], context: str) -> bool: + """Whether the job reports `context`: as its name, a matrix leg `name (…)`, or a reusable workflow's `name / …`.""" + name = _text(job.get("name")) or job_id + if "${{" in name: + return context.startswith(name.split("${{")[0]) + return context == name or context.startswith((f"{name} (", f"{name} / ")) + + +def required_check_problems(contexts: list[str], merge_group_docs: list[dict[Any, Any]]) -> list[str]: + """Explain each required context that would leave a pull request or merge group waiting.""" + problems = [] + for context in contexts: + producers = [] # (job id, skipped by the fast tier, the name it reports when skipped) + for doc in merge_group_docs: + jobs: dict[str, Any] = doc.get("jobs") or {} + skipped = _tier_skipped_jobs(jobs) + producers += [ + (job_id, job_id in skipped, _text(job.get("name")) or job_id) + for job_id, job in jobs.items() + if _reports(job_id, job, context) + ] + if not producers: + problems.append(f"Required check {context!r} comes from no workflow that triggers on merge_group, " + "so the merge queue would wait for it forever.") + # A skipped job reports one check under its own name: a skipped matrix + # job doesn't expand into its legs, and a skipped reusable-workflow + # call reports none of the called workflow's jobs. + elif all(skipped and context != name for _, skipped, name in producers): + problems.append(f"Required check {context!r} comes from job {producers[0][0]}, which the fast tier " + f"skips; a skipped job reports only its own name. Require {GATE_JOB} instead.") + return problems def required_contexts(repo: str) -> list[str] | None: @@ -170,9 +215,8 @@ def main() -> int: findings.append(Finding(str(directory), f"Exactly one workflow must define {GATE_JOB}; found {len(gate_files)}.")) if args.repo: contexts = required_contexts(args.repo) - for context in check_required_contexts(contexts or [], merge_group_docs): - findings.append(Finding(str(directory), f"Required check {context!r} comes from no workflow that " - "triggers on merge_group, so the merge queue would wait for it forever.")) + for problem in required_check_problems(contexts or [], merge_group_docs): + findings.append(Finding(str(directory), problem)) annotate = os.environ.get("GITHUB_ACTIONS") == "true" for finding in findings: diff --git a/scripts/test_check_ci_conformance.py b/scripts/test_check_ci_conformance.py index c229f4b..0ceb1a6 100644 --- a/scripts/test_check_ci_conformance.py +++ b/scripts/test_check_ci_conformance.py @@ -4,7 +4,7 @@ import yaml -from check_ci_conformance import check_required_contexts, check_workflow +from check_ci_conformance import check_workflow, required_check_problems TEMPLATE = pathlib.Path(__file__).parent.parent / "workflow-templates" / "ci.yml" @@ -109,4 +109,78 @@ def test_required_contexts_match_job_names_and_matrix_prefixes() -> None: runs-on: x """) contexts = ["ci-gate", "pre-commit (3.11)", "rust-checks (ubuntu-latest)", "fast-check"] - assert check_required_contexts(contexts, [doc]) == ["fast-check"] + problems = required_check_problems(contexts, [doc]) + assert len(problems) == 1 and "'fast-check'" in problems[0] + + +def test_required_check_from_tier_skipped_matrix_job() -> None: + doc = yaml.safe_load(""" +on: [merge_group] +jobs: + pre-commit: + needs: changes + if: needs.changes.outputs.full == 'true' + strategy: {matrix: {python-version: ['3.11']}} + runs-on: x +""") + problems = required_check_problems(["pre-commit (3.11)"], [doc]) + assert len(problems) == 1 and "Require ci-gate instead" in problems[0] + + +def test_status_function_overrides_inherited_draft_skip() -> None: + doc = template() + doc["jobs"]["test"]["if"] = "!cancelled()" + assert any("Job test must skip drafts" in m for m in messages(doc)) + + +def test_short_form_concurrency() -> None: + doc = template() + doc["concurrency"] = "ci-${{ github.event.pull_request.number }}" + assert any("cancel-in-progress" in m for m in messages(doc)) + + +def test_required_check_match_is_exact_or_expanded() -> None: + doc = yaml.safe_load(""" +on: [merge_group] +jobs: + test: {runs-on: x} +""") + problems = required_check_problems(["test-e2e"], [doc]) + assert len(problems) == 1 and "no workflow that triggers on merge_group" in problems[0] + + +def test_required_check_from_tier_skipped_reusable_workflow() -> None: + doc = yaml.safe_load(""" +on: [merge_group] +jobs: + changes: {runs-on: x} + build-test: + name: Build & Test + needs: changes + if: needs.changes.outputs.full == 'true' && needs.changes.outputs.build-test != 'false' + uses: ./.github/workflows/_test.yml +""") + problems = required_check_problems(["Build & Test / build-test (ubuntu-latest)", "changes"], [doc]) + assert len(problems) == 1 and "Build & Test / build-test" in problems[0] + + +def test_required_check_from_job_skipped_through_needs() -> None: + doc = yaml.safe_load(""" +on: [merge_group] +jobs: + changes: {runs-on: x} + build: + needs: changes + if: needs.changes.outputs.full == 'true' + runs-on: x + test: + needs: build + strategy: {matrix: {os: [a]}} + runs-on: x + report: + needs: test + if: always() + runs-on: x +""") + problems = required_check_problems(["test (a)", "report", "build"], [doc]) + assert len(problems) == 1 and "'test (a)'" in problems[0] diff --git a/workflow-templates/ci.yml b/workflow-templates/ci.yml index 30f010e..056b824 100644 --- a/workflow-templates/ci.yml +++ b/workflow-templates/ci.yml @@ -49,10 +49,13 @@ jobs: - id: mode uses: cocoindex-io/.github/actions/ci-mode@v1 # A skipped filter leaves its outputs empty, and an empty output runs - # the job: dispatched and scheduled runs test everything. + # the job: dispatched and scheduled runs test everything. So does a + # stack's top layer: on a pull request the filter lists the files the + # layer itself changed, but its run stands in for the whole stack. - id: filter if: >- steps.mode.outputs.full == 'true' + && !github.event.pull_request.stack && contains(fromJSON('["pull_request", "push", "merge_group"]'), github.event_name) uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 with: