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
commit 19a9d54e9c0c10ca5c8d7b8fa3a5288236618a96 Author: Jarek Potiuk <[email protected]> AuthorDate: Sun Oct 25 16:51:40 2026 +0100 Check newsfragment PR numbers in static checks instead of a separate workflow The separate workflow ran outside the fork CI gate (AIP-120), needed its own required check, and needed its own approval for first-time contributors. As part of the existing newsfragment hook it runs wherever static checks run, in project CI and in a contributor's fork, and fails CI passed like any other static check. A newsfragment is usually written before its pull request exists, so a local run cannot know the right number and accepts any. Generated-by: Claude Opus 5 --- .asf.yaml | 7 +- .github/workflows/basic-tests.yml | 7 ++ .github/workflows/check-newsfragment-pr-number.yml | 60 ----------- .github/workflows/ci-amd.yml | 27 +++++ .github/workflows/ci-arm.yml | 27 +++++ .github/workflows/ci-image-checks.yml | 6 ++ contributing-docs/18_contribution_workflow.rst | 6 +- dev/README_AIRFLOW3_DEV.md | 7 +- scripts/ci/prek/newsfragments.py | 61 ++++++++++++ scripts/tests/ci/prek/test_newsfragments.py | 110 ++++++++++++++++++++- 10 files changed, 247 insertions(+), 71 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index 1992824fb1f..8c62e4183ba 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -136,10 +136,9 @@ github: strict_required_status_checks_policy: false required_status_checks: - context: "CI passed" - - context: "check-newsfragment-pr-number" - # The branches `ci-amd.yml` runs pull request CI for, apart from `main`. The newsfragment check - # runs only on `main`, so these require "CI passed" alone. A release branch gets the "CI passed" - # job only when it is backported there; until then its PRs need a committer to bypass the rule. + # The branches `ci-amd.yml` runs pull request CI for, apart from `main`. A release branch gets + # the "CI passed" job only when it is backported there; until then its PRs need a committer to + # bypass the rule. - name: "Require CI on release branches" target: branch enforcement: active diff --git a/.github/workflows/basic-tests.yml b/.github/workflows/basic-tests.yml index 88740c02c46..1ecf344ef6c 100644 --- a/.github/workflows/basic-tests.yml +++ b/.github/workflows/basic-tests.yml @@ -20,6 +20,11 @@ name: Basic tests on: # yamllint disable-line rule:truthy workflow_call: inputs: + newsfragment-pr-number: + description: "Pull request the newsfragments are checked against; empty to accept any number" + required: false + type: string + default: "" runners: description: "The array of labels (in json form) determining runners." required: true @@ -316,6 +321,8 @@ jobs: PUSH_BASE_COMMIT: ${{ inputs.push-base-commit }} VERBOSE: "false" SKIP_BREEZE_PREK_HOOKS: "true" + PR_NUMBER: ${{ inputs.newsfragment-pr-number }} + GITHUB_TOKEN: ${{ github.token }} SKIP: ${{ inputs.skip-prek-hooks }} COLUMNS: "202" diff --git a/.github/workflows/check-newsfragment-pr-number.yml b/.github/workflows/check-newsfragment-pr-number.yml deleted file mode 100644 index 80dd07e33dc..00000000000 --- a/.github/workflows/check-newsfragment-pr-number.yml +++ /dev/null @@ -1,60 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -# ---- -name: Check newsfragment PR number -on: # yamllint disable-line rule:truthy - pull_request: - branches: - - main - types: [opened, reopened, synchronize, labeled, unlabeled] - -permissions: - contents: read - pull-requests: read - -concurrency: - group: check-newsfragment-${{ github.event.pull_request.number }} - cancel-in-progress: false - -jobs: - check-newsfragment-pr-number: - if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip newsfragment check') }} - runs-on: ubuntu-slim - timeout-minutes: 5 - steps: - - name: Check newsfragment PR number - env: - GH_TOKEN: ${{ github.token }} - PR_NUMBER: "${{ github.event.pull_request.number }}" - run: | - # Find newsfragment files whose PR number doesn't match this PR. - # Use the REST API to get file statuses so we can exclude deleted files. - bad=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/files" \ - --paginate --jq '.[] | select(.status != "removed") | .filename' \ - | grep '/newsfragments/.*\.rst$' \ - | grep -v "/newsfragments/${PR_NUMBER}\." || true) - - if [ -n "$bad" ]; then - msg="Newsfragment PR number mismatch." - msg+=" Expected ${PR_NUMBER} but found: ${bad}." - msg+=" Rename the newsfragment to use the correct" - msg+=" PR number, or add the 'skip newsfragment" - msg+=" check' label to the PR to skip this check." - echo "::error::${msg}" - exit 1 - fi diff --git a/.github/workflows/ci-amd.yml b/.github/workflows/ci-amd.yml index 05fa7900c0f..4bc6b138e60 100644 --- a/.github/workflows/ci-amd.yml +++ b/.github/workflows/ci-amd.yml @@ -306,6 +306,7 @@ jobs: latest-versions-only: ${{ steps.selective-checks.outputs.latest-versions-only }} mysql-exclude: ${{ steps.selective-checks.outputs.mysql-exclude }} mysql-versions: ${{ steps.selective-checks.outputs.mysql-versions }} + newsfragment-pr-number: ${{ steps.newsfragment-pr.outputs.pr-number }} # platform is hardcoded per workflow file (this is the # AMD copy). The matching ARM copy lives in ci-arm.yml. platform: "linux/amd64" @@ -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 + 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}" \ + -f state=open -f base=main --jq '.[0].number // empty' || true) + fi + if [[ "${number}" =~ ^[0-9]+$ ]]; then + echo "pr-number=${number}" >> "${GITHUB_OUTPUT}" + fi - name: Selective checks id: selective-checks env: @@ -454,6 +479,7 @@ jobs: needs: [build-info, check-run-needed] uses: ./.github/workflows/basic-tests.yml with: + newsfragment-pr-number: ${{ needs.build-info.outputs.newsfragment-pr-number }} runners: ${{ needs.build-info.outputs.runner-type }} run-ui-tests: ${{needs.build-info.outputs.run-ui-tests}} run-www-tests: ${{needs.build-info.outputs.run-www-tests}} @@ -539,6 +565,7 @@ jobs: id-token: write contents: read with: + newsfragment-pr-number: ${{ needs.build-info.outputs.newsfragment-pr-number }} runners: ${{ needs.build-info.outputs.runner-type }} platform: ${{ needs.build-info.outputs.platform }} python-versions-list-as-string: ${{ needs.build-info.outputs.python-versions-list-as-string }} diff --git a/.github/workflows/ci-arm.yml b/.github/workflows/ci-arm.yml index 1bde91c7aca..7a424c9c767 100644 --- a/.github/workflows/ci-arm.yml +++ b/.github/workflows/ci-arm.yml @@ -188,6 +188,7 @@ jobs: latest-versions-only: ${{ steps.selective-checks.outputs.latest-versions-only }} mysql-exclude: ${{ steps.selective-checks.outputs.mysql-exclude }} mysql-versions: ${{ steps.selective-checks.outputs.mysql-versions }} + newsfragment-pr-number: ${{ steps.newsfragment-pr.outputs.pr-number }} # platform is hardcoded per workflow file (this is the # ARM copy). The matching AMD copy lives in ci-amd.yml. platform: "linux/arm64" @@ -305,6 +306,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 + 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}" \ + -f state=open -f base=main --jq '.[0].number // empty' || true) + fi + if [[ "${number}" =~ ^[0-9]+$ ]]; then + echo "pr-number=${number}" >> "${GITHUB_OUTPUT}" + fi - name: Selective checks id: selective-checks env: @@ -336,6 +361,7 @@ jobs: needs: [build-info, check-run-needed] uses: ./.github/workflows/basic-tests.yml with: + newsfragment-pr-number: ${{ needs.build-info.outputs.newsfragment-pr-number }} runners: ${{ needs.build-info.outputs.runner-type }} run-ui-tests: ${{needs.build-info.outputs.run-ui-tests}} run-www-tests: ${{needs.build-info.outputs.run-www-tests}} @@ -421,6 +447,7 @@ jobs: id-token: write contents: read with: + newsfragment-pr-number: ${{ needs.build-info.outputs.newsfragment-pr-number }} runners: ${{ needs.build-info.outputs.runner-type }} platform: ${{ needs.build-info.outputs.platform }} python-versions-list-as-string: ${{ needs.build-info.outputs.python-versions-list-as-string }} diff --git a/.github/workflows/ci-image-checks.yml b/.github/workflows/ci-image-checks.yml index 1a1c06c6804..cca6c1826b4 100644 --- a/.github/workflows/ci-image-checks.yml +++ b/.github/workflows/ci-image-checks.yml @@ -20,6 +20,11 @@ name: CI Image Checks on: # yamllint disable-line rule:truthy workflow_call: inputs: + newsfragment-pr-number: + description: "Pull request the newsfragments are checked against; empty to accept any number" + required: false + type: string + default: "" runners: description: "The array of labels (in json form) determining runners." required: true @@ -154,6 +159,7 @@ jobs: SKIP: ${{ inputs.skip-prek-hooks }} COLUMNS: "202" SKIP_GROUP_OUTPUT: "true" + PR_NUMBER: ${{ inputs.newsfragment-pr-number }} DEFAULT_BRANCH: ${{ inputs.branch }} RUFF_FORMAT: "github" - name: "Show prek log on failure" diff --git a/contributing-docs/18_contribution_workflow.rst b/contributing-docs/18_contribution_workflow.rst index 1a0953d6617..b5d4fa78204 100644 --- a/contributing-docs/18_contribution_workflow.rst +++ b/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. Step 5: Pass PR Review diff --git a/dev/README_AIRFLOW3_DEV.md b/dev/README_AIRFLOW3_DEV.md index c094c9dc2fb..5b0c9af0d2e 100644 --- a/dev/README_AIRFLOW3_DEV.md +++ b/dev/README_AIRFLOW3_DEV.md @@ -195,7 +195,7 @@ Auto-merge is enabled for the whole repository, so every branch that PRs target `airflow-ctl` branches. A release branch gets the **CI passed** job only once it is backported there; until then, its PRs never report the check, and a committer merges them by bypassing the rule. -The `main` ruleset requires two checks. **CI passed** is a job in the `Tests (AMD)` workflow that always +The `main` ruleset requires one check, **CI passed**. It is a job in the `Tests (AMD)` workflow that always runs and fails when any other CI job failed; jobs that selective checks skip count as passed, so PRs that run only part of the tests still get a result. A run with cancelled jobs and no failed one reports as **CI passed (run cancelled)** instead, so a duplicate run of the same commit that concurrency cancelled @@ -206,8 +206,9 @@ hits its timeout is cancelled while the run is not, so that run's check fails, u than on PRs, has the same job, kept identical by the `check-ci-workflows-in-sync` hook. For a PR that the fork CI gate (AIP-120) leaves to CI in the contributor's fork, project CI does not run, so the job reports 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. **check-newsfragment-pr-number** runs on every -PR too. +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. 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 41704cd4bb0..d0987fd3a32 100755 --- a/scripts/ci/prek/newsfragments.py +++ b/scripts/ci/prek/newsfragments.py @@ -19,14 +19,22 @@ """ Check things about newsfragments: - Only a single line, except for `significant` changes which can have a blank line, then the body + - In CI, where ``PR_NUMBER`` is set, the newsfragments the pull request adds are named after it. + Locally the pull request may not exist yet, so any number is accepted. """ from __future__ import annotations +import json +import os +import re import sys +import urllib.request from pathlib import Path VALID_CHANGE_TYPES = {"significant", "feature", "improvement", "bugfix", "doc", "misc"} +SKIP_PR_NUMBER_LABEL = "skip newsfragment check" +NEWSFRAGMENT_PATH = re.compile(r"/newsfragments/[^/]+\.rst$") def validate_newsfragment(filename: str, lines: list[str]) -> list[str]: @@ -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: + batch = json.load(response) + items.extend(batch) + if len(batch) < 100: + return items + page += 1 + + +def check_pr_number(repo: str, 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") + 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") + 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." + for filename in find_misnumbered_newsfragments(pr_number, files) + ] + + def main(filenames: list[str]) -> int: failed = False for filename in filenames: @@ -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) + for error in errors: + print(error) + if errors: + failed = True return 1 if failed else 0 diff --git a/scripts/tests/ci/prek/test_newsfragments.py b/scripts/tests/ci/prek/test_newsfragments.py index bd249e45405..fb094bb019f 100644 --- a/scripts/tests/ci/prek/test_newsfragments.py +++ b/scripts/tests/ci/prek/test_newsfragments.py @@ -16,8 +16,18 @@ # under the License. from __future__ import annotations +import json +from unittest import mock + import pytest -from ci.prek.newsfragments import VALID_CHANGE_TYPES, validate_newsfragment +from ci.prek import newsfragments +from ci.prek.newsfragments import ( + VALID_CHANGE_TYPES, + check_pr_number, + find_misnumbered_newsfragments, + main, + validate_newsfragment, +) ALL_CHANGE_TYPES = sorted(VALID_CHANGE_TYPES) NON_SIGNIFICANT_CHANGE_TYPES = sorted(VALID_CHANGE_TYPES - {"significant"}) @@ -79,3 +89,101 @@ class TestNewsfragmentContentValidation: 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: + 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 + ] + + items = newsfragments.get_github_items("repos/apache/airflow/pulls/1/files") + + assert len(items) == 101 + requests = [call.args[0] for call in mock_urlopen.call_args_list] + assert [r.full_url for r in requests] == [ + "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)
