potiuk commented on code in PR #74819: URL: https://github.com/apache/airflow/pull/74819#discussion_r4330952080
########## dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py: ########## @@ -0,0 +1,339 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Fork CI eligibility (AIP-120). + +Decides whether a CI run in ``apache/airflow`` should use project CI or leave the pull request +to CI in the contributor's fork. See dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. + +The gate job in ``ci-amd.yml`` runs this file with the runner's ``python3`` from a sparse +checkout, without installing breeze, so it must only import from the standard library. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +import tomllib +from collections.abc import Mapping +from dataclasses import dataclass, field, replace +from datetime import UTC, datetime +from enum import Enum +from pathlib import Path +from typing import Any + +APACHE_REPO = "apache/airflow" + +LABEL_FORK_CI_REQUIRED = "fork ci required" +LABEL_USE_PROJECT_CI = "use project ci" +LABEL_USE_FORK_CI = "use fork ci" +LABEL_READY_FOR_REVIEW = "ready for maintainer review" + +DEFAULT_CLOSE_AFTER_DAYS = 7 +DEFAULT_ENABLED_SINCE = datetime(2026, 1, 1, tzinfo=UTC) + +# The push branch filters ci-amd.yml used before its push trigger became '**', as written in +# ci-arm.yml. They are GitHub filter patterns, not regular expressions: see +# `compile_branch_filter` for the difference. +CANONICAL_PUSH_BRANCH_FILTERS = ( + "v[0-9]+-[0-9]+-test", + "providers-[a-z]+-?[a-z]*/v[0-9]+-[0-9]+", + "chart/v[0-9]+-[0-9]+x-test", + "airflow-ctl/v[0-9]+-[0-9]+-test", +) + + +def compile_branch_filter(pattern: str) -> re.Pattern[str]: + """ + Compile a GitHub Actions branch filter pattern to the regular expression it means. + + In a filter, `*` matches any characters except `/` and `**` any characters at all, while `?` + and `+` make the preceding character optional or repeatable and `[...]` is a character class. + So `[a-z]*` is one letter followed by anything up to the next `/`, not a run of letters. + """ + parts = [] + index = 0 + while index < len(pattern): + char = pattern[index] + if pattern.startswith("**", index): + parts.append(".*") + index += 2 + continue + if char == "*": + parts.append("[^/]*") + elif char in "?+": + parts.append(char) + elif char == "[": + end = pattern.index("]", index) + parts.append(pattern[index : end + 1]) + index = end + 1 + continue + else: + parts.append(re.escape(char)) + index += 1 + return re.compile(f"^{''.join(parts)}$") + + +CANONICAL_PUSH_BRANCH_PATTERNS: tuple[re.Pattern[str], ...] = tuple( + compile_branch_filter(pattern) for pattern in CANONICAL_PUSH_BRANCH_FILTERS +) +PERMISSIONS_OF_COMMITTERS = frozenset({"admin", "maintain", "write"}) + +TRUSTED_ASSOCIATIONS = frozenset({"MEMBER", "OWNER", "COLLABORATOR"}) + + +class Reason(str, Enum): + SCHEDULED = "scheduled" + FORK_SCHEDULE = "fork_schedule" + FORK_PUSH = "fork_push" + CANONICAL_BRANCH = "canonical_branch" + ADHOC_BRANCH = "adhoc_branch" + LABEL_PROJECT_CI = "label_project_ci" + LABEL_FORK_CI = "label_fork_ci" + PILOT = "pilot" + GATE_DISABLED = "gate_disabled" + HEAD_IS_UPSTREAM = "head_is_upstream" + BEFORE_ENABLED_SINCE = "before_enabled_since" + TRUSTED_AUTHOR = "trusted_author" + ALLOWLISTED = "allowlisted" + EXTERNAL = "external" + + +GATED_REASONS = frozenset({Reason.EXTERNAL, Reason.PILOT, Reason.LABEL_FORK_CI}) +TAMPER_CHECKED_REASONS = frozenset({Reason.EXTERNAL, Reason.PILOT}) + + +@dataclass(frozen=True) +class GateConfig: + enabled: bool = False + enabled_since: datetime = DEFAULT_ENABLED_SINCE + close_after_days: int = DEFAULT_CLOSE_AFTER_DAYS + allowlist: Mapping[str, tuple[str, ...]] = field(default_factory=dict) + pilot: Mapping[str, datetime] = field(default_factory=dict) + + def is_allowlisted(self, login: str) -> bool: + wanted = login.casefold() + return any(wanted == member.casefold() for members in self.allowlist.values() for member in members) + + def pilot_joined_at(self, login: str) -> datetime | None: + wanted = login.casefold() + for pilot_login, joined_at in self.pilot.items(): + if pilot_login.casefold() == wanted: + return joined_at + return None + + +def _require_aware_datetime(value: Any, name: str) -> datetime: + if not isinstance(value, datetime) or value.tzinfo is None: + raise ValueError( + f"{name} must be a TOML offset date-time such as 2026-10-20T00:00:00Z, got {value!r}" + ) + return value + + +def parse_config(text: str) -> GateConfig: + try: + data = tomllib.loads(text) + except tomllib.TOMLDecodeError as e: + raise ValueError(f"Invalid fork CI gate configuration: {e}") from e + + enabled = data.get("enabled", False) + if not isinstance(enabled, bool): + raise ValueError(f"enabled must be true or false, got {enabled!r}") + enabled_since = _require_aware_datetime(data.get("enabled_since", DEFAULT_ENABLED_SINCE), "enabled_since") + close_after_days = data.get("close_after_days", DEFAULT_CLOSE_AFTER_DAYS) + if isinstance(close_after_days, bool) or not isinstance(close_after_days, int) or close_after_days < 1: + raise ValueError(f"close_after_days must be a positive integer, got {close_after_days!r}") + + allowlist: dict[str, tuple[str, ...]] = {} + for org, logins in data.get("allowlist", {}).items(): + if not isinstance(logins, list) or not all(isinstance(login, str) for login in logins): + raise ValueError(f"allowlist entry {org!r} must be a list of GitHub logins, got {logins!r}") + allowlist[org] = tuple(logins) + + pilot = { + login.casefold(): _require_aware_datetime(joined_at, f"pilot entry {login!r}") + for login, joined_at in data.get("pilot", {}).items() + } + return GateConfig( + enabled=enabled, + enabled_since=enabled_since, + close_after_days=close_after_days, + allowlist=allowlist, + pilot=pilot, + ) + + +def load_config(path: Path) -> GateConfig: + return parse_config(path.read_text()) + + +@dataclass(frozen=True) +class GateEvent: + event_name: str + repository: str + ref: str + head_repository: str = "" + author_login: str = "" + author_association: str = "" + created_at: datetime | None = None + labels: tuple[str, ...] = () + + +@dataclass(frozen=True) +class Decision: + run: bool + reason: Reason + + +def _is_canonical_branch(ref: str) -> bool: + branch = ref.removeprefix("refs/heads/") + return any(pattern.match(branch) for pattern in CANONICAL_PUSH_BRANCH_PATTERNS) + + +def decide(event: GateEvent, config: GateConfig) -> Decision: + """Apply the eligibility rules of the design in order; the first matching rule wins.""" + if event.event_name == "schedule" and event.repository != APACHE_REPO: + return Decision(run=False, reason=Reason.FORK_SCHEDULE) + if event.event_name in ("schedule", "workflow_dispatch"): + return Decision(run=True, reason=Reason.SCHEDULED) + if event.event_name == "push": + if event.repository != APACHE_REPO: + return Decision(run=True, reason=Reason.FORK_PUSH) + if _is_canonical_branch(event.ref): + return Decision(run=True, reason=Reason.CANONICAL_BRANCH) + return Decision(run=False, reason=Reason.ADHOC_BRANCH) + if LABEL_USE_PROJECT_CI in event.labels: + return Decision(run=True, reason=Reason.LABEL_PROJECT_CI) + # A branch of apache/airflow has no fork to run CI in, and its pushes are not project CI either + # (`adhoc_branch`), so `use fork ci` or a pilot entry would leave such a pull request no CI. + if event.head_repository == APACHE_REPO: + return Decision(run=True, reason=Reason.HEAD_IS_UPSTREAM) + if LABEL_USE_FORK_CI in event.labels: + return Decision(run=False, reason=Reason.LABEL_FORK_CI) + joined_at = config.pilot_joined_at(event.author_login) + if joined_at is not None and event.created_at is not None and event.created_at >= joined_at: + return Decision(run=False, reason=Reason.PILOT) + if not config.enabled: + return Decision(run=True, reason=Reason.GATE_DISABLED) + if event.created_at is not None and event.created_at < config.enabled_since: + return Decision(run=True, reason=Reason.BEFORE_ENABLED_SINCE) + if event.author_association in TRUSTED_ASSOCIATIONS: + return Decision(run=True, reason=Reason.TRUSTED_AUTHOR) + if config.is_allowlisted(event.author_login): + return Decision(run=True, reason=Reason.ALLOWLISTED) + return Decision(run=False, reason=Reason.EXTERNAL) + + +def parse_github_datetime(value: str) -> datetime | None: + if not value: + return None + return datetime.fromisoformat(value.replace("Z", "+00:00")) + + +def _run_gh(*args: str) -> str | None: + """Run ``gh`` and return its output, or None when it is missing or fails.""" + try: + result = subprocess.run(["gh", *args], capture_output=True, text=True, check=False) + except OSError as e: + print(f"Could not run gh: {e}", file=sys.stderr) + return None + if result.returncode != 0: + print(f"gh {' '.join(args)} failed: {result.stderr.strip()}", file=sys.stderr) + return None + return result.stdout + + +def fetch_author_permission(repository: str, login: str) -> str | None: Review Comment: You're right, and the mocked test couldn't show it. The lookup is gone from both the gate and the reconciler's discovery (`has_write_access` in #74822). The design doc now says a committer whose `apache` membership is private makes it public or is added to `[allowlist]`. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## scripts/ci/prek/check_fork_ci_gate_config.py: ########## @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Validate ``.github/fork-ci-gate.toml`` (AIP-120 fork CI gate). + +The file must parse with the same rules the gate job uses, and every ``[pilot]`` join instant +and ``enabled_since`` value that is new or changed relative to ``main`` must lie in the future. +The gate reads the file from the base branch, so an entry only takes effect when it merges, +while the instant is compared with pull request creation: an instant before the merge would +gate pull requests that already ran project CI. See "Contributor pilot" in +dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. +""" + +from __future__ import annotations + +import importlib.util +import os +import subprocess +import sys +from datetime import UTC, datetime +from pathlib import Path +from types import ModuleType + +REPO_ROOT = Path(__file__).resolve().parents[3] +CONFIG_PATH = ".github/fork-ci-gate.toml" +UPSTREAM_URL = "https://github.com/apache/airflow.git"; +GATE_MODULE_PATH = REPO_ROOT / "dev" / "breeze" / "src" / "airflow_breeze" / "utils" / "fork_ci_gate.py" + + +def load_gate_module() -> ModuleType: + # fork_ci_gate.py is standard-library only by design, so it can be loaded without breeze. + spec = importlib.util.spec_from_file_location("fork_ci_gate", GATE_MODULE_PATH) + if spec is None or spec.loader is None: + raise ImportError(f"Cannot load {GATE_MODULE_PATH}") + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +def find_violations(base_text: str | None, new_text: str, *, now: datetime) -> list[str]: + gate = load_gate_module() + try: + new = gate.parse_config(new_text) + except ValueError as e: + return [str(e)] + try: + base = gate.parse_config(base_text) if base_text is not None else None + except ValueError: + base = None + + violations = [] + # Moving enabled_since later (switching the gate on) must not gate pull requests opened + # before the merge. Moving it earlier is the backlog reset and gates older pull requests on + # purpose. Without a parsable base there is nothing to compare with. + if base is not None and new.enabled_since > base.enabled_since and new.enabled_since <= now: + violations.append( + f"enabled_since moved later to {new.enabled_since.isoformat()}, which is not in the future. " + "Use an instant after the expected merge." + ) + if base is not None and not base.enabled and new.enabled and new.enabled_since <= now: + violations.append( + f"enabled is switched on while enabled_since is {new.enabled_since.isoformat()}, which is not " + "in the future; that would gate pull requests that already ran project CI. Set enabled_since " + "to an instant after the expected merge in the same change." + ) + # A base branch without the file, such as a release branch the file is backported to, has no + # gate yet, so no pull request against it was ever gated and every entry takes effect only + # when the file arrives there. + if base is None: Review Comment: Fixed. The early return now checks `base_text is None` (no base file), and a base that doesn't parse checks every pilot entry as new. Added the `past-pilot-with-unparsable-base` test case. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## .github/workflows/ci-amd.yml: ########## @@ -66,12 +62,62 @@ env: VERBOSE: "true" concurrency: - group: ci-amd-${{ github.event.pull_request.number || github.ref }} + # The event name keeps push runs on main (gate-only since AIP-120) from cancelling the + # scheduled canary on the same ref. + group: ci-amd-${{ github.event.pull_request.number || format('{0}-{1}', github.event_name, github.ref) }} cancel-in-progress: true jobs: + gate: + # Fork CI gate (AIP-120): decides whether project CI runs for this event. Configuration and + # code come from the base branch so a pull request cannot change its own eligibility. See + # dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. + name: "Fork CI gate" + runs-on: ["ubuntu-slim"] + timeout-minutes: 5 + outputs: + project-ci: ${{ steps.gate.outputs.project-ci }} + reason: ${{ steps.gate.outputs.reason }} + steps: + - name: "Checkout gate configuration from the base branch" + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.base_ref || github.ref }} + fetch-depth: 1 + persist-credentials: false + sparse-checkout: | + .github/fork-ci-gate.toml + dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py + sparse-checkout-cone-mode: false + - name: "Decide whether project CI runs" Review Comment: Added. For a gated pull request, the gate job now runs `check_no_conventional_commit_message.py` (from the base branch's sparse checkout) on the title, since `ci-image-checks.yml` is skipped and the fork run is a `push`. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## .github/workflows/ci-amd.yml: ########## @@ -43,16 +43,12 @@ on: # yamllint disable-line rule:truthy - airflow-ctl/v[0-9]+-[0-9]+-stable types: [opened, reopened, synchronize, ready_for_review] push: - # Post-merge pushes to release-prep / providers branches run on both - # AMD and ARM (the matching block lives in the other wrapper too — - # `check-ci-workflows-in-sync` enforces they stay identical). `main` is - # intentionally NOT in this list: main pushes stay AMD-only via - # `ci-amd.yml`; ARM main coverage is the cron-driven canary above. + # Every push triggers the workflow so that contributors get CI in their forks (AIP-120). + # In apache/airflow the `gate` job keeps the old behaviour: only pushes to the release-prep + # / providers branches listed in ci-arm.yml (CANONICAL_PUSH_BRANCH_FILTERS in + # fork_ci_gate.py) run anything; other pushes produce a gate-only run. branches: - - v[0-9]+-[0-9]+-test - - providers-[a-z]+-?[a-z]*/v[0-9]+-[0-9]+ - - chart/v[0-9]+-[0-9]+x-test - - airflow-ctl/v[0-9]+-[0-9]+-test + - '**' Review Comment: Agreed. #74819 and #74820 will merge together, back to back, so there is no window where fork pushes run the full matrix. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## .github/workflows/ci-amd.yml: ########## @@ -43,16 +43,12 @@ on: # yamllint disable-line rule:truthy - airflow-ctl/v[0-9]+-[0-9]+-stable types: [opened, reopened, synchronize, ready_for_review] push: - # Post-merge pushes to release-prep / providers branches run on both - # AMD and ARM (the matching block lives in the other wrapper too — - # `check-ci-workflows-in-sync` enforces they stay identical). `main` is - # intentionally NOT in this list: main pushes stay AMD-only via - # `ci-amd.yml`; ARM main coverage is the cron-driven canary above. + # Every push triggers the workflow so that contributors get CI in their forks (AIP-120). + # In apache/airflow the `gate` job keeps the old behaviour: only pushes to the release-prep Review Comment: Fixed in this PR. `analyze_e2e_flaky_tests.py` now takes an `EVENT` filter that defaults to `schedule`, like `analyze_ci_job_durations.py`, with a test. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting ########## .github/workflows/ci-amd.yml: ########## @@ -66,12 +62,62 @@ env: VERBOSE: "true" concurrency: - group: ci-amd-${{ github.event.pull_request.number || github.ref }} + # The event name keeps push runs on main (gate-only since AIP-120) from cancelling the + # scheduled canary on the same ref. + group: ci-amd-${{ github.event.pull_request.number || format('{0}-{1}', github.event_name, github.ref) }} cancel-in-progress: true jobs: + gate: + # Fork CI gate (AIP-120): decides whether project CI runs for this event. Configuration and + # code come from the base branch so a pull request cannot change its own eligibility. See + # dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. + name: "Fork CI gate" + runs-on: ["ubuntu-slim"] + timeout-minutes: 5 + outputs: + project-ci: ${{ steps.gate.outputs.project-ci }} + reason: ${{ steps.gate.outputs.reason }} + steps: + - name: "Checkout gate configuration from the base branch" + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.base_ref || github.ref }} + fetch-depth: 1 + persist-credentials: false + sparse-checkout: | + .github/fork-ci-gate.toml + dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py + sparse-checkout-cone-mode: false + - name: "Decide whether project CI runs" + id: gate + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GATE_CONFIG: .github/fork-ci-gate.toml + GATE_SCRIPT: dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + PR_AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} + PR_CREATED_AT: ${{ github.event.pull_request.created_at }} + PR_EVENT_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} + # A base branch with neither gate file predates the gate (the pull request adding it, + # or a release branch cut before it) and keeps running project CI. With only one of + # them present, the missing file fails the job instead of opening the gate. + run: | + if [[ ! -e "${GATE_CONFIG}" && ! -e "${GATE_SCRIPT}" ]]; then Review Comment: Fixed at the source: for a `push` in apache/airflow, the gate now checks out the gate files from the default branch instead of the pushed ref. A push's decision depends only on the ref, so a push to `v3-3-stable` keeps being skipped (`adhoc_branch`), and a pre-gate release branch like `v3-1-test` still runs as `canonical_branch`. A push in a fork keeps the pushed commit's gate. A second review caught that the fork's default branch belongs to the contributor: a gate there that skips every job would give a green fork run that nothing in the PR shows. A gate change in the pushed commit is in the PR's files, so the run is `green_unverified`. The design doc explains both. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
