Andrushika commented on code in PR #75612:
URL: https://github.com/apache/airflow/pull/75612#discussion_r4371476032
##########
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:
We might need a different instruction here for fork CI. A maintainer cannot
re-run jobs in the contributor's fork.
##########
.github/workflows/ci-amd.yml:
##########
@@ -423,6 +424,30 @@ jobs:
env:
SKIP_BREEZE_SELF_UPGRADE_CHECK: "true"
GITHUB_CONTEXT_INPUT: "${{ runner.temp }}/github_context.json"
+ # The newsfragment check of static checks fails a newsfragment of the
pull request that is
+ # named after another number. A push in a fork (AIP-120) has no pull
request in its event,
+ # so the open pull request of the pushed branch is looked up; without
one, any number passes.
Review Comment:
We might need to check the newsfragment numbers in the fork CI reconciler
too. The first fork run starts before the PR exists; that run would accept any
number.
##########
.github/workflows/ci-amd.yml:
##########
@@ -423,6 +424,30 @@ jobs:
env:
SKIP_BREEZE_SELF_UPGRADE_CHECK: "true"
GITHUB_CONTEXT_INPUT: "${{ runner.temp }}/github_context.json"
+ # The newsfragment check of static checks fails a newsfragment of the
pull request that is
+ # named after another number. A push in a fork (AIP-120) has no pull
request in its event,
+ # so the open pull request of the pushed branch is looked up; without
one, any number passes.
+ - name: "Find the pull request for the newsfragment check"
+ id: newsfragment-pr
+ env:
+ GH_TOKEN: ${{ github.token }}
+ EVENT_NAME: ${{ github.event_name }}
+ BASE_REF: ${{ github.base_ref }}
+ EVENT_PR_NUMBER: ${{ github.event.pull_request.number }}
+ REPOSITORY: ${{ github.repository }}
+ OWNER: ${{ github.repository_owner }}
+ REF_NAME: ${{ github.ref_name }}
+ run: |
+ number=""
+ if [[ "${EVENT_NAME}" == "pull_request" && "${BASE_REF}" == "main"
]]; then
Review Comment:
Nit: We might need `&& "${REPOSITORY}" == "apache/airflow"` here and in
`ci-arm.yml`. A `pull_request` run inside a fork has the fork's PR number.
##########
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:
Nit: We might need to catch `HTTPError` and `URLError` here. Now an API
error prints a full traceback.
##########
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:
Nit: We might need the expected header in the params, not an `if token:`
branch.
##########
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:
Nit: We might not need `PR_REPOSITORY`. No workflow sets it.
--
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]
