diff --git a/contributing-docs/18_contribution_workflow.rst b/contributing-docs/18_contribution_workflow.rst index 89a73df97128c..65e953ef04d58 100644 --- a/contributing-docs/18_contribution_workflow.rst +++ b/contributing-docs/18_contribution_workflow.rst @@ -147,15 +147,17 @@ Set up your checkout and fork once: 2. If fork CI applies to you, enable the ``Tests (AMD)`` workflow in your fork: today that means you joined the pilot or a maintainer labelled your pull request ``use fork ci``; once fork CI is switched on for everyone, it means you are not a committer, a collaborator or a member of an - exempted organisation. GitHub disables workflows in new forks. Otherwise keep it disabled, so - your pushes do not run CI twice: + exempted organisation. GitHub disables workflows in new forks. Sync your fork's ``main`` with + ``apache/airflow`` first, so it has the current version of the workflow: .. code-block:: bash + gh repo sync /airflow --branch main gh workflow enable ci-amd.yml -R /airflow - If the workflow is missing, your fork's ``main`` is older than it; sync it first with - ``gh repo sync /airflow --branch main``. + Keep the workflow enabled for as long as you have open pull requests. If it gets disabled, from + your fork's Actions tab or by GitHub after 60 days without activity in your fork, the fork CI + monitor comments on your pull requests with the commands to turn it back on. 3. Check everything at once. ``breeze ci audit`` prints one row per check and the command that fixes anything missing; ``--fix`` runs those commands after asking: diff --git a/dev/breeze/doc/08_ci_tasks.rst b/dev/breeze/doc/08_ci_tasks.rst index d54eadbaa694d..08171e760398b 100644 --- a/dev/breeze/doc/08_ci_tasks.rst +++ b/dev/breeze/doc/08_ci_tasks.rst @@ -168,8 +168,8 @@ Pull requests from external contributors can run their CI in the contributor's f ``breeze ci audit`` checks, read-only, whether your checkout and fork are set up for that: ``gh`` installed and authenticated, the ``upstream`` remote pointing at ``apache/airflow`` and ``origin`` at your fork, whether you are a committer, collaborator, member of an exempted organisation, pilot participant or -external contributor, whether the ``Tests (AMD)`` workflow in your fork is enabled (or disabled, for those -whose pull requests run project CI), how far your fork's ``main`` is behind, and the branch you are on. +external contributor, whether the ``Tests (AMD)`` workflow in your fork is enabled (when your pull requests +use fork CI), how far your fork's ``main`` is behind, and the branch you are on. Each failing check prints the command that fixes it; ``--fix`` runs those commands after asking. These are all available flags of ``audit`` command: diff --git a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md index ab33c09aec374..ad06dbc54c504 100644 --- a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md +++ b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md @@ -490,16 +490,19 @@ that matters. It is matched by file, not by the workflow's display name, which a change, and only `push` runs count because only they execute the workflow from the head commit itself; a `pull_request` run inside the fork, into the fork's own `main`, runs a merge the contributor controls. For pull requests -whose recorded state is `setup_required` (or that have no recorded state yet), one REST call -per pull request reads `GET /repos//actions/workflows/ci-amd.yml` to distinguish -"workflows disabled" from "enabled but not yet pushed". This is bounded by the number of -pull requests in that state, not by the total. +with no fork run for the head commit, one REST call per pull request reads +`GET /repos//actions/workflows/ci-amd.yml` to distinguish "workflows disabled" from +"enabled but not yet pushed". It is read on every run, including for pull requests already +recorded as `awaiting_push`: a workflow can be disabled after it was enabled, by the +contributor or by GitHub after 60 days without activity in the fork, and the pull request +would otherwise keep waiting for a push that never runs CI. This is bounded by the number of +pull requests without a fork run for their head commit, not by the total. ### State machine | Observed fork state | Draft | `ready for maintainer review` | Commit status | Comment | |---|---|---|---|---| -| **setup_required**: workflow state is `disabled_fork`, or the fork has no `ci-amd.yml` | draft | remove | `error` (red), link to the fork's Actions page for `ci-amd.yml` | create or update: how to enable, with the exact `gh workflow enable ci-amd.yml -R ` line, and `breeze ci audit` | +| **setup_required**: workflow state is `disabled_fork`, `disabled_manually` or `disabled_inactivity`, or the fork has no `ci-amd.yml` | draft | remove | `error` (red), link to the fork's Actions page for `ci-amd.yml` | create or update: how to enable, with the exact `gh repo sync --branch main` and `gh workflow enable ci-amd.yml -R ` lines, and `breeze ci audit`; on transition from any other state but `closed` (the workflow stopped running after it worked): also post a **new** comment saying why it stopped | | **awaiting_push**: enabled, no run for the head SHA | draft | remove | `pending` (yellow), link to the fork's Actions page | on transition from setup_required: post a **new** comment asking to push again (an edit would not notify) | | **running**: run in progress | draft | remove | `pending` (yellow), link to the run | update state and run link | | **failed**: conclusion is failure, cancelled or timed out | draft | remove | `failure` (red), link to the run | update with the run link | @@ -872,7 +875,7 @@ Unknown means any of: that outlasted its retries. - A check suite for `Tests (AMD)`, in the fork or in `apache/airflow`, with a status or conclusion outside the modelled set. -- A workflow state outside `active` / `disabled_fork` / `disabled_manually` / not found. +- A workflow state outside `active` / `disabled_fork` / `disabled_manually` / `disabled_inactivity` / not found. - A mutation that returned an error (label, comment, close, status). - A head repository that no longer exists. - A state comment whose marker cannot be parsed. @@ -949,18 +952,17 @@ prints one row: status, finding, and the command that fixes it. | 2 | Remote naming | a remote named `upstream` points at `apache/airflow` and `origin` does not | `git remote rename`/`add` per `contributing-docs/10_working_with_git.rst` | | 3 | Fork exists | `origin` is a fork of `apache/airflow` owned by the authenticated user | `gh repo fork apache/airflow --remote=false` then add `origin`, unless check 2's fix already adds or repoints it | | 4 | Who you are | classifies the user in the gate's order: pilot participant (`[pilot]`, which gates whatever the role), committer (public `apache` membership, `GET /orgs/apache/public_members/`, since the gate cannot see private membership), collaborator (`.asf.yaml`), exempted (`[allowlist]`), external; an external user whose `apache` membership is private is told to make it public or ask for `[allowlist]`; when the membership call fails for any reason but a 404, the role is reported as not checked and checks 2, 3 and 5 stay informational | informational | -| 5 | Fork workflows | external or pilot participant: `ci-amd.yml` state on the fork is `active`; committer/collaborator/exempted: informational, with a hint when it is `active` | `gh workflow enable ci-amd.yml -R `, or `gh workflow disable ...` shown but not run by `--fix` | +| 5 | Fork workflows | external or pilot participant: `ci-amd.yml` state on the fork is `active`; committer/collaborator/exempted: always OK, whatever the state | `gh workflow enable ci-amd.yml -R ` | | 6 | Fork `main` freshness | fork `main` is not behind `upstream/main` by more than 200 commits (`MAX_FORK_MAIN_BEHIND`) | `gh repo sync --branch main` | | 7 | Current branch | not `main`, has an upstream tracking `origin` | informational | The role is determined before checks 2 and 3, though printed as check 4. For a committer, collaborator or exempted contributor, whose pull requests run project CI, checks 2 and 3 are informational: working from an `apache/airflow` clone without a fork is fine for them, and -`--fix` only runs the fixes of failed checks. The same goes for check 5: an enabled fork -workflow runs every push twice and attaches a second status, so they are told they can disable -it, but it is not a failure, because a committer trying fork CI with the `use fork ci` label -needs it enabled. Forks of a public repository cannot be made private on GitHub, so disabling -the workflow is the only recommendation available. +`--fix` only runs the fixes of failed checks. Check 5 passes for them whether the fork workflow +is enabled or not. The audit never suggests disabling it: a committer trying fork CI with the +`use fork ci` label needs it enabled, and a workflow disabled while a pull request waits for its +fork CI leaves that pull request stuck until the workflow is enabled again. A failed API call is reported as "could not check" rather than as a finding: a failed fork lookup is not "not a fork of apache/airflow", and a failed workflow lookup is not "missing". diff --git a/dev/breeze/src/airflow_breeze/utils/ci_audit.py b/dev/breeze/src/airflow_breeze/utils/ci_audit.py index 3b27ef8eb15ed..94b0cf7ce9d7b 100644 --- a/dev/breeze/src/airflow_breeze/utils/ci_audit.py +++ b/dev/breeze/src/airflow_breeze/utils/ci_audit.py @@ -327,18 +327,6 @@ def run_audit(*, run: Runner, config: GateConfig, collaborators: frozenset[str]) enable, ) ) - elif required is False and state == "active": - # Not a failure: a committer trying fork CI with the `use fork ci` label needs it on. - results.append( - CheckResult( - 5, - "fork workflows", - CheckStatus.INFO, - f"`Tests (AMD)` is enabled in {fork}, but your pull requests run project CI, so each " - "push runs CI twice; disable it unless you use the `use fork ci` label", - (f"gh workflow disable {CI_WORKFLOW_FILE} -R {fork}",), - ) - ) elif required is None: results.append( CheckResult( diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py index 48208ae90a1e4..90f6b6cba51c7 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py @@ -116,11 +116,12 @@ def fork_actions_url(fork: str) -> str: def _setup_instructions(fork: str) -> str: return ( "To get CI for this pull request:\n\n" - "1. Enable the workflow in your fork (once per fork):\n\n" + "1. Sync your fork's `main` with apache/airflow, so that it has the current version of the " + "workflow:\n\n" + f" ```bash\n gh repo sync {fork} --branch main\n ```\n\n" + "2. Enable the workflow in your fork:\n\n" f" ```bash\n gh workflow enable ci-amd.yml -R {fork}\n ```\n\n" - " If your fork's `main` is older than the workflow, sync it first with " - f"`gh repo sync {fork} --branch main`.\n" - "2. Push your branch again. Every push then runs CI in your fork.\n\n" + "3. Push your branch again. Every push then runs CI in your fork.\n\n" "`breeze ci audit` checks your whole local and fork setup and prints the command that fixes " f"anything missing. See [the contribution workflow]({CONTRIBUTING_URL})." ) @@ -195,6 +196,23 @@ def nudge_comment(*, fork: str) -> str: ) +_DISABLED_REASONS = { + "disabled_manually": "was disabled in your fork, from its Actions tab or with `gh workflow disable`", + "disabled_inactivity": "was disabled by GitHub because your fork had no activity for 60 days", + "disabled_fork": "is disabled in your fork", + "missing": "is missing from your fork, for example because its `main` was reset to an older commit", +} + + +def workflow_disabled_comment(*, fork: str, workflow_state: str | None) -> str: + reason = _DISABLED_REASONS.get(workflow_state or "missing", "is not enabled in your fork") + return ( + f"**Fork CI stopped running.** The `Tests (AMD)` workflow {reason}, so pushes to this " + "branch no longer run CI, and this pull request stays a draft until they do.\n\n" + f"{_setup_instructions(fork)}" + FOOTER + ) + + def inactivity_close_comment(*, days: int, fork: str) -> str: return ( f"Closing this pull request because CI has not run in your fork for {days} days. {WHY_FORK_CI}\n\n" diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py index 6b54f6c7fac26..cd9edf5a44c3c 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -73,6 +73,7 @@ status_description, tamper_close_comment, ungated_comment, + workflow_disabled_comment, ) CI_WORKFLOW_FILE = "ci-amd.yml" @@ -144,7 +145,7 @@ RUNNING_STATUSES = frozenset({"QUEUED", "IN_PROGRESS", "PENDING", "WAITING", "REQUESTED"}) FAILED_CONCLUSIONS = frozenset({"FAILURE", "CANCELLED", "TIMED_OUT", "STARTUP_FAILURE", "ACTION_REQUIRED"}) -SETUP_WORKFLOW_STATES = frozenset({"disabled_fork", "disabled_manually", "missing"}) +SETUP_WORKFLOW_STATES = frozenset({"disabled_fork", "disabled_inactivity", "disabled_manually", "missing"}) class ForkState(str, Enum): @@ -284,8 +285,6 @@ def observe_fork_state(fork: ForkObservation, marker: Marker | None) -> tuple[Fo return ForkState.FAILED, None return None, f"unexpected fork check suite conclusion {fork.suite_conclusion!r}" if fork.workflow_state is None: - if marker is not None and marker.state == ForkState.AWAITING_PUSH.value: - return ForkState.AWAITING_PUSH, None return None, "no fork run and the fork workflow state was not read" if fork.workflow_state in SETUP_WORKFLOW_STATES: return ForkState.SETUP_REQUIRED, None @@ -294,10 +293,6 @@ def observe_fork_state(fork: ForkObservation, marker: Marker | None) -> tuple[Fo return None, f"unexpected fork workflow state {fork.workflow_state!r}" -def needs_workflow_state(marker: Marker | None) -> bool: - return marker is None or marker.state != ForkState.AWAITING_PUSH.value - - def has_job_started(job: dict[str, Any]) -> bool: # A job that never reached a runner -- queued, or cancelled while queued -- has no steps and a # `runner_id` of null or 0, but its `status` can be "completed" and its `started_at` set. @@ -587,6 +582,17 @@ def release(state: ForkState, comment: Callable[[Marker], str]) -> Plan: # tracked at https://github.com/apache/airflow/issues/74908 if marker and marker.state == ForkState.SETUP_REQUIRED.value and state == ForkState.AWAITING_PUSH: plan.new_comments.append(nudge_comment(fork=pr.head_repo)) + # A new comment, because the edited state comment would not notify the author that fork CI they + # had working stopped running. Like the nudge, it is not retried when posting it fails; tracked at + # https://github.com/apache/airflow/issues/74908 + if ( + marker + and marker.state not in (ForkState.SETUP_REQUIRED.value, ForkState.CLOSED.value) + and state == ForkState.SETUP_REQUIRED + ): + plan.new_comments.append( + workflow_disabled_comment(fork=pr.head_repo, workflow_state=fork.workflow_state) + ) if state in CLOSABLE_STATES and now - since >= timedelta(days=config.close_after_days): plan.state = ForkState.CLOSED @@ -1501,12 +1507,9 @@ def run_reconcile( if pr.head_status_unread: pr = read_head_status(client, repo, pr) fork = observations.get(pr.number, ForkObservation()) - if ( - pr.head_repo - and fork.suite_status is None - and fork.error is None - and needs_workflow_state(pr.marker) - ): + # Read on every run, not only until the workflow is first seen enabled: it can be + # disabled later, and a pull request waiting for a push would then wait forever. + if pr.head_repo and fork.suite_status is None and fork.error is None: fork = ForkObservation(workflow_state=fetch_workflow_state(client, pr.head_repo)) jobs_reported = None if config.job_statuses: diff --git a/dev/breeze/tests/test_ci_audit.py b/dev/breeze/tests/test_ci_audit.py index 2dbc0d8a3a548..9697a67acd04d 100644 --- a/dev/breeze/tests/test_ci_audit.py +++ b/dev/breeze/tests/test_ci_audit.py @@ -260,8 +260,8 @@ def test_fork_problems(repo, message): frozenset(), (0, "active"), Role.COMMITTER, - CheckStatus.INFO, - ("gh workflow disable ci-amd.yml -R someone/airflow",), + CheckStatus.OK, + (), id="committer-enabled-is-not-a-failure", ), pytest.param( diff --git a/dev/breeze/tests/test_fork_ci_messages.py b/dev/breeze/tests/test_fork_ci_messages.py index 407065ca4a049..0c238cff94954 100644 --- a/dev/breeze/tests/test_fork_ci_messages.py +++ b/dev/breeze/tests/test_fork_ci_messages.py @@ -42,6 +42,7 @@ render_marker, state_comment, tamper_close_comment, + workflow_disabled_comment, ) MARKER = Marker(state="awaiting_push", sha="abc123", since=datetime(2026, 10, 15, 10, 0, tzinfo=UTC)) @@ -114,6 +115,9 @@ def test_parse_marker_malformed(body): pytest.param(nudge_comment(fork="u/airflow"), id="nudge"), pytest.param(inactivity_close_comment(days=7, fork="u/airflow"), id="inactivity"), pytest.param(tamper_close_comment(run_url="https://x"), id="tamper"), + pytest.param( + workflow_disabled_comment(fork="u/airflow", workflow_state="disabled_manually"), id="disabled" + ), ], ) def test_every_comment_ends_with_footer(text): @@ -125,9 +129,11 @@ def test_every_comment_ends_with_footer(text): [ pytest.param(state_comment(Marker("setup_required", "a", MARKER.since), fork="u/fork", run_url=None)), pytest.param(inactivity_close_comment(days=7, fork="u/fork")), + pytest.param(workflow_disabled_comment(fork="u/fork", workflow_state="disabled_manually")), ], ) def test_setup_texts_give_the_exact_command(text): + assert "gh repo sync u/fork --branch main" in text assert "gh workflow enable ci-amd.yml -R u/fork" in text diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 9f3aa61f751d0..38977df88b441 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -141,12 +141,19 @@ def _classify(pr, fork=None, *, bypass_run=None, changes_ci_files=False, config= id="enabled-no-run", ), pytest.param( - ForkObservation(), + ForkObservation(workflow_state="active"), _marker("awaiting_push"), ForkState.AWAITING_PUSH, "pending", id="still-awaiting", ), + pytest.param( + ForkObservation(workflow_state="disabled_inactivity"), + None, + ForkState.SETUP_REQUIRED, + "error", + id="workflow-disabled-for-inactivity", + ), pytest.param( ForkObservation(suite_status="IN_PROGRESS", run_url=RUN_URL), None, @@ -451,6 +458,37 @@ def test_nudge_on_transition_from_setup_required(): assert "Push this branch again" in plan.new_comments[0] +@pytest.mark.parametrize( + ("marker", "workflow_state", "expected_reason"), + [ + pytest.param( + _marker("awaiting_push"), "disabled_manually", "was disabled in your fork", id="awaiting-push" + ), + pytest.param(_marker("green"), "disabled_inactivity", "no activity for 60 days", id="green"), + pytest.param(_marker("failed"), "missing", "is missing from your fork", id="failed"), + ], +) +def test_comment_when_workflow_disabled_after_it_worked(marker, workflow_state, expected_reason): + plan, _, _ = _classify(_pr(marker=marker), ForkObservation(workflow_state=workflow_state)) + + assert plan.state == ForkState.SETUP_REQUIRED + assert len(plan.new_comments) == 1 + assert "Fork CI stopped running" in plan.new_comments[0] + assert expected_reason in plan.new_comments[0] + assert "gh workflow enable ci-amd.yml -R someone/airflow" in plan.new_comments[0] + + +@pytest.mark.parametrize( + "marker", + [pytest.param(None, id="first-run"), pytest.param(_marker("setup_required"), id="still-disabled")], +) +def test_no_disabled_comment_when_workflow_never_worked(marker): + plan, _, _ = _classify(_pr(marker=marker), ForkObservation(workflow_state="disabled_fork")) + + assert plan.state == ForkState.SETUP_REQUIRED + assert plan.new_comments == [] + + @pytest.mark.parametrize( ("marker", "should_close"), [ @@ -1501,6 +1539,40 @@ def rest_get(path, paginate=False, items_key=None): assert client.set_status.call_args.args[:3] == (2, "sha2", "error") +def test_run_reconcile_rereads_workflow_state_while_awaiting_push(client, tmp_path): + marker_comment = { + "databaseId": 5, + "body": render_marker(_marker("awaiting_push", sha="sha1")), + "author": {"login": "github-actions"}, + } + client.graphql.side_effect = [ + _search_result(), + _search_result(), + _search_result(_pr_node(1, comments={"nodes": [marker_comment]})), + {"pr0": {"object": None}}, + ] + client.rest_get_pages.return_value = [{"total_count": 0, "workflow_runs": []}] + + def rest_get(path, paginate=False, items_key=None): + if path == "repos/someone/my-airflow/actions/workflows/ci-amd.yml": + return {"state": "disabled_manually"} + raise AssertionError(path) + + client.rest_get.side_effect = rest_get + + run_reconcile( + client, + CONFIG, + now=NOW, + run_url="https://run", + output_file=tmp_path / "s.json", + workflows_dir=tmp_path, + ) + + assert client.set_status.call_args.args[:3] == (1, "sha1", "error") + assert "Fork CI stopped running" in client.create_comment.call_args.args[1] + + @pytest.mark.parametrize( ("state", "deletes"), [