This is an automated email from the ASF dual-hosted git repository. vatsrahul1001 pushed a commit to branch v3-4-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 7a48d57b1cf915d7159b5d09f4ec0be86e4e94ba Author: Shahar Epstein <[email protected]> AuthorDate: Fri Nov 6 11:02:38 2026 +0200 Stop labelling fork CI PRs that change CI files for workflow approval (#76289) (#76582) 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. (cherry picked from commit 4c110e645d02d12afbe5177a97008485ae157f9c) Co-authored-by: Andrew Chang <[email protected]> --- .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 f956ee81c4d..ebc42af64fc 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 299180b5364..781da587905 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")])
