This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch fold-newsfragment-check
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/fold-newsfragment-check by
this push:
new 2baad18536a Address review of the newsfragment PR number check
2baad18536a is described below
commit 2baad18536a81e6cc949a2d352d7bde01b9b2197
Author: Jarek Potiuk <[email protected]>
AuthorDate: Sun Oct 25 17:23:39 2026 +0100
Address review of the newsfragment PR number check
A pull request inside a fork carries the fork's number, so only
apache/airflow pull requests are checked. A GitHub API error is reported
without a traceback, and the instructions say who re-runs the checks when CI
runs in the contributor's fork.
Generated-by: Claude Opus 5
---
.github/workflows/ci-amd.yml | 4 +-
.github/workflows/ci-arm.yml | 4 +-
contributing-docs/18_contribution_workflow.rst | 4 +-
dev/README_AIRFLOW3_DEV.md | 3 +-
scripts/ci/prek/newsfragments.py | 18 +++++---
scripts/tests/ci/prek/test_newsfragments.py | 62 +++++++++++++++++---------
6 files changed, 64 insertions(+), 31 deletions(-)
diff --git a/.github/workflows/ci-amd.yml b/.github/workflows/ci-amd.yml
index 4bc6b138e60..cd3d5f95eb2 100644
--- a/.github/workflows/ci-amd.yml
+++ b/.github/workflows/ci-amd.yml
@@ -439,7 +439,9 @@ jobs:
REF_NAME: ${{ github.ref_name }}
run: |
number=""
- if [[ "${EVENT_NAME}" == "pull_request" && "${BASE_REF}" == "main"
]]; then
+ # A pull request inside a fork has a number of the fork's, not of
apache/airflow.
+ if [[ "${EVENT_NAME}" == "pull_request" && "${BASE_REF}" == "main" \
+ && "${REPOSITORY}" == "apache/airflow" ]]; then
number="${EVENT_PR_NUMBER}"
elif [[ "${EVENT_NAME}" == "push" && "${REPOSITORY}" !=
"apache/airflow" ]]; then
number=$(gh api -X GET repos/apache/airflow/pulls -f
head="${OWNER}:${REF_NAME}" \
diff --git a/.github/workflows/ci-arm.yml b/.github/workflows/ci-arm.yml
index 7a424c9c767..f10c1f07716 100644
--- a/.github/workflows/ci-arm.yml
+++ b/.github/workflows/ci-arm.yml
@@ -321,7 +321,9 @@ jobs:
REF_NAME: ${{ github.ref_name }}
run: |
number=""
- if [[ "${EVENT_NAME}" == "pull_request" && "${BASE_REF}" == "main"
]]; then
+ # A pull request inside a fork has a number of the fork's, not of
apache/airflow.
+ if [[ "${EVENT_NAME}" == "pull_request" && "${BASE_REF}" == "main" \
+ && "${REPOSITORY}" == "apache/airflow" ]]; then
number="${EVENT_PR_NUMBER}"
elif [[ "${EVENT_NAME}" == "push" && "${REPOSITORY}" !=
"apache/airflow" ]]; then
number=$(gh api -X GET repos/apache/airflow/pulls -f
head="${OWNER}:${REF_NAME}" \
diff --git a/contributing-docs/18_contribution_workflow.rst
b/contributing-docs/18_contribution_workflow.rst
index b5d4fa78204..d864040f24c 100644
--- a/contributing-docs/18_contribution_workflow.rst
+++ b/contributing-docs/18_contribution_workflow.rst
@@ -323,7 +323,9 @@ summary and a body separated by a blank line, similar to
``git`` commit messages
Static checks in CI validate that newsfragment filenames use the correct PR
number. Run locally, before the
PR exists, they accept any number. If you need to skip this check (e.g. when
cherry-picking a newsfragment
-from another PR), add the ``skip newsfragment check`` label to your PR and
re-run the failed jobs.
+from another PR), ask a maintainer to add the ``skip newsfragment check``
label to your PR, then re-run the
+failed static checks. When your PR's CI runs in your fork, re-run them there,
or push again; maintainers
+cannot re-run jobs in your fork.
Step 5: Pass PR Review
diff --git a/dev/README_AIRFLOW3_DEV.md b/dev/README_AIRFLOW3_DEV.md
index 5b0c9af0d2e..9307d68953c 100644
--- a/dev/README_AIRFLOW3_DEV.md
+++ b/dev/README_AIRFLOW3_DEV.md
@@ -208,7 +208,8 @@ fork CI gate (AIP-120) leaves to CI in the contributor's
fork, project CI does n
as **CI passed (project CI not run)** instead, and the fork CI reconciler
posts **CI passed** once the
fork run for the PR's latest commit is green and trusted. Static checks also
fail a newsfragment of a PR
to `main` that is named after another PR number, in project CI and in the fork
run; the
-`skip newsfragment check` label turns that off, after a re-run of the failed
jobs.
+`skip newsfragment check` label turns that off, after a re-run of the failed
static checks, which for CI
+in the contributor's fork the contributor does there.
Auto-merge does not wait for checks from other workflows: **CodeQL**,
**Dependency review** and
**Registry tests**. The last two only run when dependency or registry files
change. Look at those
diff --git a/scripts/ci/prek/newsfragments.py b/scripts/ci/prek/newsfragments.py
index d0987fd3a32..69a2f0883cc 100755
--- a/scripts/ci/prek/newsfragments.py
+++ b/scripts/ci/prek/newsfragments.py
@@ -29,11 +29,13 @@ import json
import os
import re
import sys
+import urllib.error
import urllib.request
from pathlib import Path
VALID_CHANGE_TYPES = {"significant", "feature", "improvement", "bugfix",
"doc", "misc"}
SKIP_PR_NUMBER_LABEL = "skip newsfragment check"
+PR_REPOSITORY = "apache/airflow"
NEWSFRAGMENT_PATH = re.compile(r"/newsfragments/[^/]+\.rst$")
@@ -98,21 +100,22 @@ def get_github_items(path: str) -> list[dict]:
page += 1
-def check_pr_number(repo: str, pr_number: str) -> list[str]:
+def check_pr_number(pr_number: str) -> list[str]:
"""Return the errors for the newsfragments the pull request adds under
another number."""
- labels = get_github_items(f"repos/{repo}/issues/{pr_number}/labels")
+ labels =
get_github_items(f"repos/{PR_REPOSITORY}/issues/{pr_number}/labels")
if any(label["name"] == SKIP_PR_NUMBER_LABEL for label in labels):
print(f"Newsfragment PR number not checked: the pull request has the
{SKIP_PR_NUMBER_LABEL!r} label.")
return []
# Removed files are left out, so deleting a misnumbered newsfragment fixes
the check.
files = [
f["filename"]
- for f in get_github_items(f"repos/{repo}/pulls/{pr_number}/files")
+ for f in
get_github_items(f"repos/{PR_REPOSITORY}/pulls/{pr_number}/files")
if f["status"] != "removed"
]
return [
f"Newsfragment {filename} is not named after pull request {pr_number}.
Rename it to "
- f"{pr_number}.<type>.rst, or add the {SKIP_PR_NUMBER_LABEL!r} label
and re-run the failed jobs."
+ f"{pr_number}.<type>.rst, or have a maintainer add the
{SKIP_PR_NUMBER_LABEL!r} label and "
+ "re-run the failed static checks (with CI in your fork, re-run them
there)."
for filename in find_misnumbered_newsfragments(pr_number, files)
]
@@ -129,7 +132,12 @@ def main(filenames: list[str]) -> int:
failed = True
pr_number = os.environ.get("PR_NUMBER")
if pr_number:
- errors = check_pr_number(os.environ.get("PR_REPOSITORY",
"apache/airflow"), pr_number)
+ try:
+ errors = check_pr_number(pr_number)
+ except (urllib.error.HTTPError, urllib.error.URLError) as e:
+ errors = [
+ f"Could not read pull request {pr_number} from GitHub to check
newsfragment numbers: {e}"
+ ]
for error in errors:
print(error)
if errors:
diff --git a/scripts/tests/ci/prek/test_newsfragments.py
b/scripts/tests/ci/prek/test_newsfragments.py
index fb094bb019f..c43b522352e 100644
--- a/scripts/tests/ci/prek/test_newsfragments.py
+++ b/scripts/tests/ci/prek/test_newsfragments.py
@@ -17,6 +17,8 @@
from __future__ import annotations
import json
+import urllib.error
+from email.message import Message
from unittest import mock
import pytest
@@ -122,7 +124,7 @@ class TestNewsfragmentPrNumber:
],
]
- errors = check_pr_number("apache/airflow", "123")
+ errors = check_pr_number("123")
assert len(errors) == 1
assert "airflow-core/newsfragments/124.bugfix.rst" in errors[0]
@@ -135,44 +137,60 @@ class TestNewsfragmentPrNumber:
def test_check_pr_number_is_skipped_by_label(self, mock_get_items):
mock_get_items.return_value = [{"name": "skip newsfragment check"}]
- assert check_pr_number("apache/airflow", "123") == []
+ assert check_pr_number("123") == []
mock_get_items.assert_called_once()
@pytest.mark.parametrize(
- ("env", "expected_call", "expected_exit"),
+ ("pr_number", "expected_calls", "expected_exit"),
[
- pytest.param({}, None, 0, id="local-run-accepts-any-number"),
- pytest.param({"PR_NUMBER": "123"}, mock.call("apache/airflow",
"123"), 1, id="ci"),
- pytest.param(
- {"PR_NUMBER": "123", "PR_REPOSITORY": "someone/airflow"},
- mock.call("someone/airflow", "123"),
- 1,
- id="ci-other-repository",
- ),
+ pytest.param(None, [], 0, id="local-run-accepts-any-number"),
+ pytest.param("123", [mock.call("123")], 1, id="ci"),
],
)
@mock.patch.object(newsfragments, "check_pr_number", autospec=True,
return_value=["wrong number"])
def test_main_checks_pr_number_only_in_ci(
- self, mock_check, monkeypatch, tmp_path, env, expected_call,
expected_exit
+ self, mock_check, monkeypatch, tmp_path, pr_number, expected_calls,
expected_exit
):
- monkeypatch.delenv("PR_NUMBER", raising=False)
- monkeypatch.delenv("PR_REPOSITORY", raising=False)
- for key, value in env.items():
- monkeypatch.setenv(key, value)
+ if pr_number:
+ monkeypatch.setenv("PR_NUMBER", pr_number)
+ else:
+ monkeypatch.delenv("PR_NUMBER", raising=False)
newsfragment = tmp_path / "newsfragments" / "124.bugfix.rst"
newsfragment.parent.mkdir()
newsfragment.write_text("A change\n")
assert main([str(newsfragment)]) == expected_exit
- assert mock_check.call_args_list == ([expected_call] if expected_call
else [])
+ assert mock_check.call_args_list == expected_calls
- @pytest.mark.parametrize("token", [None, "secret"])
+ @pytest.mark.parametrize(
+ "error",
+ [
+ pytest.param(urllib.error.URLError("no network"), id="url-error"),
+ pytest.param(
+ urllib.error.HTTPError("https://api.github.com";, 403, "rate
limited", Message(), None),
+ id="http-error",
+ ),
+ ],
+ )
+ @mock.patch.object(newsfragments, "check_pr_number", autospec=True)
+ def test_main_reports_an_unreadable_pull_request(self, mock_check,
monkeypatch, capsys, error):
+ monkeypatch.setenv("PR_NUMBER", "123")
+ mock_check.side_effect = error
+
+ assert main([]) == 1
+ assert "Could not read pull request 123 from GitHub" in
capsys.readouterr().out
+
+ @pytest.mark.parametrize(
+ ("token", "expected_authorization"),
+ [pytest.param(None, None, id="anonymous"), pytest.param("secret",
"Bearer secret", id="token")],
+ )
@mock.patch.object(newsfragments.urllib.request, "urlopen", autospec=True)
- def test_get_github_items_reads_every_page(self, mock_urlopen,
monkeypatch, token):
+ def test_get_github_items_reads_every_page(
+ self, mock_urlopen, monkeypatch, token, expected_authorization
+ ):
+ monkeypatch.delenv("GITHUB_TOKEN", raising=False)
if token:
monkeypatch.setenv("GITHUB_TOKEN", token)
- else:
- monkeypatch.delenv("GITHUB_TOKEN", raising=False)
pages = [[{"n": i} for i in range(100)], [{"n": 100}]]
mock_urlopen.return_value.__enter__.side_effect = [
mock.Mock(read=mock.Mock(return_value=json.dumps(page).encode()))
for page in pages
@@ -186,4 +204,4 @@ class TestNewsfragmentPrNumber:
"https://api.github.com/repos/apache/airflow/pulls/1/files?per_page=100&page=1";,
"https://api.github.com/repos/apache/airflow/pulls/1/files?per_page=100&page=2";,
]
- assert requests[0].get_header("Authorization") == (f"Bearer {token}"
if token else None)
+ assert requests[0].get_header("Authorization") ==
expected_authorization
