This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new 4c110e645d0 Stop labelling fork CI PRs that change CI files for
workflow approval (#76289)
4c110e645d0 is described below
commit 4c110e645d02d12afbe5177a97008485ae157f9c
Author: Andrew Chang <[email protected]>
AuthorDate: Mon Nov 2 23:43:55 2026 +0800
Stop labelling fork CI PRs that change CI files for workflow approval
(#76289)
A pull request whose CI runs in the contributor's fork gets its
project CI runs held when it changes CI definition files. It got the
"need workflow approval" label for that, and "needs project ci" once
its fork run was green. Without "use project ci", approving the held
runs does not start project CI, so the first label asked maintainers
for a step that does not help. It also stayed on pull requests whose
fork run failed.
---
.github/workflows/auto-approve-workflow-runs.yml | 8 ++---
contributing-docs/25_maintainer_pr_triage.md | 2 +-
dev/breeze/doc/08_ci_tasks.rst | 4 +--
dev/breeze/doc/ci/05_workflows.md | 4 +--
.../doc/ci/08_fork_ci_for_external_contributors.md | 9 ++---
.../airflow_breeze/utils/workflow_run_approval.py | 15 +++++++--
dev/breeze/tests/test_workflow_run_approval.py | 38 ++++++++++++++++++++++
7 files changed, 64 insertions(+), 16 deletions(-)
diff --git a/.github/workflows/auto-approve-workflow-runs.yml
b/.github/workflows/auto-approve-workflow-runs.yml
index ef635daf243..876ab4756eb 100644
--- a/.github/workflows/auto-approve-workflow-runs.yml
+++ b/.github/workflows/auto-approve-workflow-runs.yml
@@ -19,10 +19,10 @@
# Approves the waiting workflow runs of external contributors' PRs to main
when the change is safe to
# run without a maintainer's look, and labels the others "need workflow
approval" with a pending
# "Workflow approval" commit status saying why. A PR whose CI runs in the
contributor's fork (AIP-120)
-# is approved unless it changes CI definition files, and then also gets a
comment saying why its runs
-# wait. When the runs start, mark-workflow-runs-approved.yml removes the label
and turns the status
-# green. PRs from the repositories of trusted stakeholders (Amazon, Google,
Astronomer) are always
-# approved. The rules are in
+# is approved unless it changes CI definition files, and then gets a comment
saying why its runs wait
+# instead of the label. When the runs start, mark-workflow-runs-approved.yml
removes the label and
+# turns the status green. PRs from the repositories of trusted stakeholders
(Amazon, Google,
+# Astronomer) are always approved. The rules are in
# dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py and documented
in
# contributing-docs/05_pull_requests.rst.
#
diff --git a/contributing-docs/25_maintainer_pr_triage.md
b/contributing-docs/25_maintainer_pr_triage.md
index 3d873e17942..ddbc647895d 100644
--- a/contributing-docs/25_maintainer_pr_triage.md
+++ b/contributing-docs/25_maintainer_pr_triage.md
@@ -238,7 +238,7 @@ The triage workflow uses the following labels and states:
| Label / State | Meaning |
|---|---|
| `ready for maintainer review` | PR has passed Stage 1 and is queued for
human review. Applied only after the skill has verified that no workflow runs
are awaiting approval. |
-| `need workflow approval` | The PR's CI workflow runs wait for a maintainer,
because the `Auto approve workflow runs` workflow did not approve them: a PR to
a branch other than `main`, a change to CI definition files, a full-tests or
dependency change, a change whose CI jobs are estimated at more than 310
job-minutes, or a change of more than 100 lines of source code (see [Pull
Requests](05_pull_requests.rst)). For a PR whose CI runs in the contributor's
fork, only a change to CI defini [...]
+| `need workflow approval` | The PR's CI workflow runs wait for a maintainer,
because the `Auto approve workflow runs` workflow did not approve them: a PR to
a branch other than `main`, a change to CI definition files, a full-tests or
dependency change, a change whose CI jobs are estimated at more than 310
job-minutes, or a change of more than 100 lines of source code (see [Pull
Requests](05_pull_requests.rst)). The workflow also tries to add the label when
processing the PR failed; its [...]
| `closed because of multiple quality violations` | PR was closed because the
author has multiple open PRs with quality issues. |
| `suspicious changes detected` | PR (and all open PRs by the same author) was
closed because the diff contained suspicious patterns (secret exfiltration,
malicious CI modifications, etc.). |
| `closed because of open PR limit` | PR was closed in the one-time closure
that introduced the [open PR limit](32_open_pull_request_limit.rst). Applied by
`dev/close_prs_over_open_pr_limit.py`, never by the triage skill. If the author
reopens the PR, triage it like any other. |
diff --git a/dev/breeze/doc/08_ci_tasks.rst b/dev/breeze/doc/08_ci_tasks.rst
index 36eb3743eb4..7b7760f3bc4 100644
--- a/dev/breeze/doc/08_ci_tasks.rst
+++ b/dev/breeze/doc/08_ci_tasks.rst
@@ -209,8 +209,8 @@ than 310 job-minutes, that change at most 100 lines of
source code and that do n
approves the runs and they start, the ``mark-workflow-runs-approved`` workflow
removes the label and
turns the status green. PRs whose CI runs in
the contributor's fork (AIP-120) only run the fork CI gate in
``apache/airflow``, so their runs are
-approved whatever their size unless they change CI definition files; such a PR
also gets a comment
-explaining that its CI change needs a maintainer. The runs of PRs from the
repositories of trusted
+approved whatever their size unless they change CI definition files; such a PR
gets a comment
+instead of the label, explaining that its CI change needs a maintainer. The
runs of PRs from the repositories of trusted
stakeholders (Amazon, Google, Astronomer) are always approved. For every PR it
also
estimates the CI jobs selective checks will run and how long they take, and
writes them to the job
summary. Use ``--report-only`` to see the decisions without approving runs or
changing labels or
diff --git a/dev/breeze/doc/ci/05_workflows.md
b/dev/breeze/doc/ci/05_workflows.md
index a371d363004..bfb56b73dd6 100644
--- a/dev/breeze/doc/ci/05_workflows.md
+++ b/dev/breeze/doc/ci/05_workflows.md
@@ -124,8 +124,8 @@ them, for the contributors the repository's approval
setting covers. The
A PR to `main` whose CI runs in the contributor's fork (AIP-120: the `Fork CI
gate` job would skip
project CI for it) is decided by one rule instead, because its `Tests (AMD)`
run only executes the
gate: its runs are approved whatever its size, cost or dependency changes,
unless it changes one of
-those CI-defining files. Such a PR gets the `need workflow approval` label and
one comment telling
-the author that the CI change needs a maintainer; see
+those CI-defining files. Such a PR gets one comment instead of the `need
workflow approval` label,
+telling the author that the CI change needs a maintainer; see
[Approving gated
runs](08_fork_ci_for_external_contributors.md#approving-gated-runs).
The runs of PRs whose head repository belongs to a trusted stakeholder
(`astronomer/airflow` for
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 e4efe527cd3..06b4a88741f 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
@@ -385,13 +385,14 @@ maintainer. One rule applies instead:
"Trusting a green fork run"). The approved `ci-amd.yml` run executes only
the gate, and the other
`pull_request` workflows (CodeQL for the changed languages, the newsfragment
check, dependency
review) run on GitHub-hosted runners.
-- Otherwise the runs wait for a maintainer, the pull request is labelled `need
workflow approval`,
- its head commit gets a pending `Workflow approval` status naming the CI
files (as for every
- pull request whose runs wait), and the workflow posts one comment explaining
that the change to the listed CI files needs a
+- Otherwise the runs wait for a maintainer, its head commit gets a pending
`Workflow approval`
+ status naming the CI files (as for every pull request whose runs wait), and
the workflow posts one comment explaining that the change to the listed CI
files needs a
maintainer to review it and approve the runs. The comment starts with the
marker
`<!-- workflow-approval: ci-change -->` and is posted once per pull request;
only comments by
`github-actions[bot]` with the marker count. A change to `.github/` can
remove the gate or add a
- workflow that does run, so such a run must not start without a review.
+ workflow that does run, so such a run must not start without a review. The
pull request does
+ not get the `need workflow approval` label, and loses it if it has it.
Approving its runs does
+ not start project CI. A maintainer adds `use project ci` for that.
A run started before `use fork ci` or `use project ci` was last added or
removed is approved too,
so a committer can add `use fork ci` after the PR's first runs were created
and before they are
diff --git a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py
b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py
index d32d0455f1b..2dba4b3842a 100644
--- a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py
+++ b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py
@@ -168,6 +168,8 @@ class ApprovalDecision:
estimate: CiMinutesEstimate | None = None
# Posted on the PR once, when the runs are left for a maintainer.
comment: str | None = None
+ # Whether a PR whose runs are left for a maintainer gets the ``need
workflow approval`` label.
+ add_approval_label: bool = True
# The labels the decision depends on: runs started before one of them
changed are not approved.
decision_labels: frozenset[str] = TEST_SELECTION_LABELS
# Runs started before this are not approved either.
@@ -253,6 +255,8 @@ def decide_gated_approval(*, changed_files:
list[ChangedFile]) -> ApprovalDecisi
approve=False,
reasons=[f"its CI runs in the fork, but it changes CI definition
files: {', '.join(ci_files)}"],
comment=ci_change_approval_comment(ci_files=ci_files),
+ # Approving does not start project CI; a maintainer adds `use
project ci` for that.
+ add_approval_label=False,
)
return ApprovalDecision(
approve=True,
@@ -871,15 +875,18 @@ def apply_decision(
return
# Reasons can contain contributor-chosen file names, which must not be
read as Rich markup.
console_print(f"[warning]PR #{pr_number} needs a maintainer: {escape(';
'.join(decision.reasons))}[/]")
- if not has_label:
+ label_added = decision.add_approval_label and not has_label
+ if label_added:
api.add_label(pr_number, NEED_WORKFLOW_APPROVAL_LABEL)
+ elif has_label and not decision.add_approval_label:
+ api.remove_label(pr_number, NEED_WORKFLOW_APPROVAL_LABEL)
status_changed = update_approval_status(
api,
sha=pr["head"]["sha"],
state="pending",
description=build_status_description("Waits for a maintainer",
decision.reasons),
)
- if (not has_label or status_changed) and clear_if_runs_were_approved(api,
pr=pr, run_ids=run_ids):
+ if (label_added or status_changed) and clear_if_runs_were_approved(api,
pr=pr, run_ids=run_ids):
return
# Once per PR: every later push would otherwise repeat it while the change
stays.
if decision.comment is not None and not api.has_bot_comment(pr_number,
CI_CHANGE_MARKER):
@@ -917,8 +924,10 @@ def write_step_summary(results: list[PullRequestResult],
github_repository: str)
decision = "failed"
elif result.decision.approve:
decision = "approved"
- else:
+ elif result.decision.add_approval_label:
decision = f"`{NEED_WORKFLOW_APPROVAL_LABEL}`"
+ else:
+ decision = "waits for a maintainer"
lines.append(
f"|
[#{result.pr_number}](https://github.com/{github_repository}/pull/{result.pr_number})
"
f"| `{result.head_sha[:10]}` | {decision} |
{format_estimate(result.decision.estimate)} "
diff --git a/dev/breeze/tests/test_workflow_run_approval.py
b/dev/breeze/tests/test_workflow_run_approval.py
index 6aa5d85d5ee..86c9511cc74 100644
--- a/dev/breeze/tests/test_workflow_run_approval.py
+++ b/dev/breeze/tests/test_workflow_run_approval.py
@@ -494,6 +494,27 @@ def
test_write_step_summary_shows_failed_pull_requests_as_failed(tmp_path, monke
assert NEED_WORKFLOW_APPROVAL_LABEL not in text
+def
test_write_step_summary_does_not_show_the_label_on_a_pull_request_left_without_it(tmp_path,
monkeypatch):
+ summary = tmp_path / "summary.md"
+ monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary))
+
+ write_step_summary(
+ [
+ PullRequestResult(
+ pr_number=10,
+ head_sha="abcdef0123456789",
+ run_ids=[1],
+ decision=ApprovalDecision(approve=False, reasons=["it changes
CI"], add_approval_label=False),
+ )
+ ],
+ "apache/airflow",
+ )
+
+ text = summary.read_text()
+ assert "| `abcdef0123` | waits for a maintainer |" in text
+ assert NEED_WORKFLOW_APPROVAL_LABEL not in text
+
+
@pytest.mark.parametrize(
("paths", "approve"),
[
@@ -900,6 +921,7 @@ def test_decide_for_pull_request_gated_to_fork_ci(
else:
assert decision.comment is not None
assert all(f"`{path}`" in decision.comment for path in ci_files)
+ assert decision.add_approval_label is False
@pytest.mark.parametrize(
@@ -1295,6 +1317,22 @@ def
test_apply_decision_does_not_recheck_the_runs_when_nothing_was_written():
api.set_approval_status.assert_not_called()
[email protected](
+ ("labels", "expected_remove_label_calls"),
+ [((), []), ((NEED_WORKFLOW_APPROVAL_LABEL,), [mock.call(10,
NEED_WORKFLOW_APPROVAL_LABEL)])],
+)
+def test_apply_decision_without_the_approval_label_removes_it(labels,
expected_remove_label_calls):
+ api = mock.MagicMock(spec=GitHubApi)
+ api.get_approval_status.return_value = _status("pending", "Waits for a
maintainer: it changes CI")
+ decision = ApprovalDecision(approve=False, reasons=["it changes CI"],
add_approval_label=False)
+
+ apply_decision(api=api, pr=_pr(10, "aaa", labels=labels), run_ids=[1],
decision=decision)
+
+ api.add_label.assert_not_called()
+ assert api.remove_label.call_args_list == expected_remove_label_calls
+ api.is_run_waiting_for_approval.assert_not_called()
+
+
@mock.patch(f"{MODULE}.decide_for_pull_request", autospec=True)
def
test_process_waiting_runs_clears_a_failed_pull_request_whose_runs_were_approved_meanwhile(mock_decide):
api = _api_with_prs([_pr(10, "aaa")], [_run(1, "aaa")])
