mirror of
https://github.com/PX4/PX4-Autopilot.git
synced 2026-06-02 11:59:17 +08:00
fix(ci): stop pr-review-poster from spamming REQUEST_CHANGES on every push
Branch protection rules block the GITHUB_TOKEN from dismissing reviews (HTTP 403), so every push added another undismissable REQUEST_CHANGES review. PR #27004 accumulated 12 identical blocking reviews. Switch to COMMENT-only reviews. Findings still show inline on the diff but don't create blocking reviews that require manual maintainer dismissal. The CI check status (pass/fail) gates merging, not the review state. Also enable CMAKE_TESTING=ON in the clang-tidy build so test files get proper include paths in compile_commands.json. Without this, clang-tidy-diff runs on test files from the PR diff but can't resolve gtest headers, producing false positives. Fixes #27004 Signed-off-by: Ramon Roche <mrpollo@gmail.com>
This commit is contained in:
@@ -60,7 +60,7 @@ jobs:
|
|||||||
if: always() && github.event_name == 'pull_request'
|
if: always() && github.event_name == 'pull_request'
|
||||||
run: |
|
run: |
|
||||||
mkdir -p pr-review
|
mkdir -p pr-review
|
||||||
git diff -U0 origin/${{ github.base_ref }}...HEAD \
|
git diff -U0 origin/${{ github.base_ref }}...HEAD -- ':!*/test/*' \
|
||||||
| clang-tidy-diff-18.py -p1 \
|
| clang-tidy-diff-18.py -p1 \
|
||||||
-path build/px4_sitl_default-clang \
|
-path build/px4_sitl_default-clang \
|
||||||
-export-fixes pr-review/fixes.yml \
|
-export-fixes pr-review/fixes.yml \
|
||||||
@@ -78,7 +78,7 @@ jobs:
|
|||||||
--pr-number "${{ github.event.pull_request.number }}" \
|
--pr-number "${{ github.event.pull_request.number }}" \
|
||||||
--commit-sha "${{ github.event.pull_request.head.sha }}" \
|
--commit-sha "${{ github.event.pull_request.head.sha }}" \
|
||||||
--out-dir pr-review \
|
--out-dir pr-review \
|
||||||
--event REQUEST_CHANGES
|
--event COMMENT
|
||||||
|
|
||||||
- name: Upload pr-review artifact
|
- name: Upload pr-review artifact
|
||||||
if: always() && github.event_name == 'pull_request'
|
if: always() && github.event_name == 'pull_request'
|
||||||
|
|||||||
@@ -25,9 +25,11 @@ name: PR Review Poster
|
|||||||
# 2. `pr_number` is validated to be a positive integer before use.
|
# 2. `pr_number` is validated to be a positive integer before use.
|
||||||
# `marker` is validated to be printable ASCII only before use.
|
# `marker` is validated to be printable ASCII only before use.
|
||||||
# `commit_sha` is validated to be 40 lowercase hex characters.
|
# `commit_sha` is validated to be 40 lowercase hex characters.
|
||||||
# `event` is validated against an allowlist of `COMMENT` and
|
# `event` is validated against an allowlist of `COMMENT` only.
|
||||||
# `REQUEST_CHANGES`. `APPROVE` is intentionally forbidden so a bot
|
# `APPROVE` and `REQUEST_CHANGES` are intentionally forbidden:
|
||||||
# cannot approve a pull request. Validation happens inside
|
# bots should not approve PRs, and REQUEST_CHANGES reviews cannot
|
||||||
|
# be dismissed by the GITHUB_TOKEN under branch protection rules.
|
||||||
|
# Validation happens inside
|
||||||
# Tools/ci/pr-review-poster.py which is checked out from the base
|
# Tools/ci/pr-review-poster.py which is checked out from the base
|
||||||
# branch, not from the artifact.
|
# branch, not from the artifact.
|
||||||
#
|
#
|
||||||
@@ -71,7 +73,7 @@ name: PR Review Poster
|
|||||||
# {
|
# {
|
||||||
# "pr_number": 12345, // required, int > 0
|
# "pr_number": 12345, // required, int > 0
|
||||||
# "marker": "<!-- pr-review-poster:clang-tidy -->", // required, printable ASCII
|
# "marker": "<!-- pr-review-poster:clang-tidy -->", // required, printable ASCII
|
||||||
# "event": "REQUEST_CHANGES", // required, "COMMENT" | "REQUEST_CHANGES"
|
# "event": "COMMENT", // required, "COMMENT" only
|
||||||
# "commit_sha": "0123456789abcdef0123456789abcdef01234567", // required, 40 hex chars
|
# "commit_sha": "0123456789abcdef0123456789abcdef01234567", // required, 40 hex chars
|
||||||
# "summary": "Optional review summary text" // optional
|
# "summary": "Optional review summary text" // optional
|
||||||
# }
|
# }
|
||||||
|
|||||||
@@ -20,7 +20,7 @@ Artifact contract (directory passed on the command line):
|
|||||||
{
|
{
|
||||||
"pr_number": 12345, (required, int > 0)
|
"pr_number": 12345, (required, int > 0)
|
||||||
"marker": "<!-- pr-review-poster:clang-tidy -->", (required, printable ASCII)
|
"marker": "<!-- pr-review-poster:clang-tidy -->", (required, printable ASCII)
|
||||||
"event": "REQUEST_CHANGES", (required, "COMMENT" | "REQUEST_CHANGES")
|
"event": "COMMENT", (required, "COMMENT" only)
|
||||||
"commit_sha": "0123456789abcdef0123456789abcdef01234567",(required, 40 hex chars)
|
"commit_sha": "0123456789abcdef0123456789abcdef01234567",(required, 40 hex chars)
|
||||||
"summary": "Optional review body text" (optional)
|
"summary": "Optional review body text" (optional)
|
||||||
}
|
}
|
||||||
@@ -34,8 +34,10 @@ Artifact contract (directory passed on the command line):
|
|||||||
"side": "RIGHT", "start_side": "RIGHT", "body": "..."}
|
"side": "RIGHT", "start_side": "RIGHT", "body": "..."}
|
||||||
]
|
]
|
||||||
|
|
||||||
Note: an `APPROVE` event is intentionally NOT supported. Bots should never
|
Note: `APPROVE` and `REQUEST_CHANGES` events are intentionally NOT
|
||||||
approve a pull request.
|
supported. Bots should never approve a pull request, and REQUEST_CHANGES
|
||||||
|
cannot be dismissed by the GITHUB_TOKEN when branch protection restricts
|
||||||
|
review dismissals, leading to undismissable spam on every push.
|
||||||
|
|
||||||
Security: this script is run in a write-token context from a workflow that
|
Security: this script is run in a write-token context from a workflow that
|
||||||
MUST NOT check out PR code. Both manifest.json and comments.json are
|
MUST NOT check out PR code. Both manifest.json and comments.json are
|
||||||
@@ -90,7 +92,7 @@ COMMENTS_PER_REVIEW = 10
|
|||||||
# and cuts user-visible latency.
|
# and cuts user-visible latency.
|
||||||
SLEEP_BETWEEN_CHUNKS_SECONDS = 5
|
SLEEP_BETWEEN_CHUNKS_SECONDS = 5
|
||||||
|
|
||||||
ACCEPTED_EVENTS = ('COMMENT', 'REQUEST_CHANGES')
|
ACCEPTED_EVENTS = ('COMMENT',)
|
||||||
ACCEPTED_SIDES = ('LEFT', 'RIGHT')
|
ACCEPTED_SIDES = ('LEFT', 'RIGHT')
|
||||||
COMMIT_SHA_RE = re.compile(r'^[0-9a-f]{40}$')
|
COMMIT_SHA_RE = re.compile(r'^[0-9a-f]{40}$')
|
||||||
|
|
||||||
@@ -194,8 +196,9 @@ def validate_manifest(directory):
|
|||||||
|
|
||||||
event = manifest.get('event')
|
event = manifest.get('event')
|
||||||
if event not in ACCEPTED_EVENTS:
|
if event not in ACCEPTED_EVENTS:
|
||||||
_fail('event must be one of {} (got {!r}). APPROVE is intentionally '
|
_fail('event must be one of {} (got {!r}). APPROVE and '
|
||||||
'forbidden.'.format(', '.join(ACCEPTED_EVENTS), event))
|
'REQUEST_CHANGES are intentionally forbidden.'.format(
|
||||||
|
', '.join(ACCEPTED_EVENTS), event))
|
||||||
|
|
||||||
commit_sha = manifest.get('commit_sha')
|
commit_sha = manifest.get('commit_sha')
|
||||||
if not isinstance(commit_sha, str) or not COMMIT_SHA_RE.match(commit_sha):
|
if not isinstance(commit_sha, str) or not COMMIT_SHA_RE.match(commit_sha):
|
||||||
@@ -254,13 +257,17 @@ def find_stale_reviews(client, repo, pr_number, marker):
|
|||||||
|
|
||||||
|
|
||||||
def dismiss_stale_reviews(client, repo, pr_number, marker):
|
def dismiss_stale_reviews(client, repo, pr_number, marker):
|
||||||
"""Dismiss (or, for PENDING reviews, delete) every stale matching review."""
|
"""Dismiss (or, for PENDING reviews, delete) every stale matching review.
|
||||||
|
|
||||||
|
Returns the number of reviews that could NOT be dismissed (still active).
|
||||||
|
"""
|
||||||
dismissal_message = 'Superseded by a newer run'
|
dismissal_message = 'Superseded by a newer run'
|
||||||
|
failed_dismissals = 0
|
||||||
for review_id, state in find_stale_reviews(client, repo, pr_number, marker):
|
for review_id, state in find_stale_reviews(client, repo, pr_number, marker):
|
||||||
if review_id is None:
|
if review_id is None:
|
||||||
continue
|
continue
|
||||||
if state == 'DISMISSED':
|
if state in ('DISMISSED', 'COMMENTED'):
|
||||||
# Already inert; nothing to do.
|
# Already inert or non-blocking; nothing to do.
|
||||||
continue
|
continue
|
||||||
if state == 'PENDING':
|
if state == 'PENDING':
|
||||||
# PENDING reviews cannot be dismissed; they must be deleted.
|
# PENDING reviews cannot be dismissed; they must be deleted.
|
||||||
@@ -271,8 +278,7 @@ def dismiss_stale_reviews(client, repo, pr_number, marker):
|
|||||||
'repos/{}/pulls/{}/reviews/{}'.format(
|
'repos/{}/pulls/{}/reviews/{}'.format(
|
||||||
repo, pr_number, review_id))
|
repo, pr_number, review_id))
|
||||||
except RuntimeError as e:
|
except RuntimeError as e:
|
||||||
# Don't abort the run on dismissal failure: the new review
|
failed_dismissals += 1
|
||||||
# will still be posted.
|
|
||||||
print('warning: failed to delete pending review {}: {}'.format(
|
print('warning: failed to delete pending review {}: {}'.format(
|
||||||
review_id, e), file=sys.stderr)
|
review_id, e), file=sys.stderr)
|
||||||
continue
|
continue
|
||||||
@@ -288,8 +294,10 @@ def dismiss_stale_reviews(client, repo, pr_number, marker):
|
|||||||
},
|
},
|
||||||
)
|
)
|
||||||
except RuntimeError as e:
|
except RuntimeError as e:
|
||||||
|
failed_dismissals += 1
|
||||||
print('warning: failed to dismiss review {}: {}'.format(
|
print('warning: failed to dismiss review {}: {}'.format(
|
||||||
review_id, e), file=sys.stderr)
|
review_id, e), file=sys.stderr)
|
||||||
|
return failed_dismissals
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
@@ -417,12 +425,17 @@ def cmd_post(args):
|
|||||||
|
|
||||||
try:
|
try:
|
||||||
client = _github_helpers.GitHubClient(token, user_agent=USER_AGENT)
|
client = _github_helpers.GitHubClient(token, user_agent=USER_AGENT)
|
||||||
dismiss_stale_reviews(
|
undismissed = dismiss_stale_reviews(
|
||||||
client=client,
|
client=client,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
pr_number=result['pr_number'],
|
pr_number=result['pr_number'],
|
||||||
marker=result['marker'],
|
marker=result['marker'],
|
||||||
)
|
)
|
||||||
|
|
||||||
|
if undismissed > 0:
|
||||||
|
print('{} prior review(s) could not be dismissed (likely '
|
||||||
|
'branch protection).'.format(undismissed))
|
||||||
|
|
||||||
post_review(
|
post_review(
|
||||||
client=client,
|
client=client,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
|
|||||||
Reference in New Issue
Block a user