potiuk commented on code in PR #75612:
URL: https://github.com/apache/airflow/pull/75612#discussion_r4371517501
##########
contributing-docs/18_contribution_workflow.rst:
##########
@@ -321,9 +321,9 @@ and place it in `airflow-core/newsfragments
</airflow-core/newsfragments>`__ for
`chart/newsfragments </chart/newsfragments>`__ for helm chart newsfragments.
It can be a single line, or a
summary and a body separated by a blank line, similar to ``git`` commit
messages.
-A CI check validates that newsfragment filenames use the correct PR 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.
+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.
Review Comment:
Good point, fixed: a maintainer adds the label, and with CI in the
contributor's fork the contributor re-runs the static checks there or pushes
again. Same wording in the hook's error message.
---
Drafted-by: Claude Opus 5; reviewed by @potiuk before posting
##########
scripts/ci/prek/newsfragments.py:
##########
@@ -63,6 +71,52 @@ def validate_newsfragment(filename: str, lines: list[str])
-> list[str]:
return errors
+def find_misnumbered_newsfragments(pr_number: str, filenames: list[str]) ->
list[str]:
+ return [
+ filename
+ for filename in filenames
+ if NEWSFRAGMENT_PATH.search(filename) and not
Path(filename).name.startswith(f"{pr_number}.")
+ ]
+
+
+def get_github_items(path: str) -> list[dict]:
+ """Return every item of a paginated GitHub REST API listing."""
+ headers = {"Accept": "application/vnd.github+json"}
+ if token := os.environ.get("GITHUB_TOKEN"):
+ headers["Authorization"] = f"Bearer {token}"
+ items: list[dict] = []
+ page = 1
+ while True:
+ request = urllib.request.Request(
+ f"https://api.github.com/{path}?per_page=100&page={page}";,
headers=headers
+ )
+ with urllib.request.urlopen(request, timeout=30) as response:
Review Comment:
Fixed: `HTTPError`/`URLError` are reported as a one-line error and fail the
check.
---
Drafted-by: Claude Opus 5; reviewed by @potiuk before posting
##########
scripts/ci/prek/newsfragments.py:
##########
@@ -73,6 +127,13 @@ def main(filenames: list[str]) -> int:
print(error)
if errors:
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)
Review Comment:
Removed; the repository is a constant now.
---
Drafted-by: Claude Opus 5; reviewed by @potiuk before posting
##########
scripts/tests/ci/prek/test_newsfragments.py:
##########
@@ -79,3 +89,101 @@ def test_significant_many_lines_ok(self):
lines = ["Big change", "", "Details here", "More details", "Even more"]
errors = validate_newsfragment("123.significant.rst", lines)
assert errors == []
+
+
+class TestNewsfragmentPrNumber:
+ @pytest.mark.parametrize(
+ ("filenames", "expected"),
+ [
+ pytest.param(["airflow-core/newsfragments/123.bugfix.rst"], [],
id="matching"),
+ pytest.param(
+ ["airflow-core/newsfragments/124.bugfix.rst"],
+ ["airflow-core/newsfragments/124.bugfix.rst"],
+ id="other-number",
+ ),
+ pytest.param(
+ ["airflow-core/newsfragments/1234.bugfix.rst"],
+ ["airflow-core/newsfragments/1234.bugfix.rst"],
+ id="number-with-the-same-prefix",
+ ),
+ pytest.param(["airflow-core/docs/124.bugfix.rst", "README.md"],
[], id="not-a-newsfragment"),
+ ],
+ )
+ def test_find_misnumbered_newsfragments(self, filenames, expected):
+ assert find_misnumbered_newsfragments("123", filenames) == expected
+
+ @mock.patch.object(newsfragments, "get_github_items", autospec=True)
+ def test_check_pr_number_ignores_removed_files(self, mock_get_items):
+ mock_get_items.side_effect = [
+ [{"name": "area:core"}],
+ [
+ {"filename": "airflow-core/newsfragments/124.bugfix.rst",
"status": "added"},
+ {"filename": "airflow-core/newsfragments/99.bugfix.rst",
"status": "removed"},
+ ],
+ ]
+
+ errors = check_pr_number("apache/airflow", "123")
+
+ assert len(errors) == 1
+ assert "airflow-core/newsfragments/124.bugfix.rst" in errors[0]
+ assert mock_get_items.call_args_list == [
+ mock.call("repos/apache/airflow/issues/123/labels"),
+ mock.call("repos/apache/airflow/pulls/123/files"),
+ ]
+
+ @mock.patch.object(newsfragments, "get_github_items", autospec=True)
+ 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") == []
+ mock_get_items.assert_called_once()
+
+ @pytest.mark.parametrize(
+ ("env", "expected_call", "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",
+ ),
+ ],
+ )
+ @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
+ ):
+ monkeypatch.delenv("PR_NUMBER", raising=False)
+ monkeypatch.delenv("PR_REPOSITORY", raising=False)
+ for key, value in env.items():
+ monkeypatch.setenv(key, value)
+ 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 [])
+
+ @pytest.mark.parametrize("token", [None, "secret"])
+ @mock.patch.object(newsfragments.urllib.request, "urlopen", autospec=True)
+ def test_get_github_items_reads_every_page(self, mock_urlopen,
monkeypatch, token):
+ if token:
Review Comment:
Done, the expected header is a parameter now.
---
Drafted-by: Claude 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]
