Skip to content

Commit f1b7bbf

Browse files
rluvatonrluvaton
committed
build: allow posting comments on forked PRs
1 parent 73ca6a5 commit f1b7bbf

3 files changed

Lines changed: 173 additions & 93 deletions

File tree

‎.github/workflows/breaking_changes_detector.yml‎

Lines changed: 41 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,24 @@
2020
# Only public workspace crates that have file changes are checked.
2121
# Internal crates (benchmarks, test-utils, sqllogictest, doc) are excluded.
2222
#
23-
# If breaking changes are found, a sticky comment is posted on the PR.
24-
# The comment is removed automatically once the issues are resolved.
23+
# This workflow only runs cargo-semver-checks and uploads the result as an
24+
# artifact. The actual PR comment is posted by a companion workflow
25+
# (`breaking_changes_detector_comment.yml`) that picks up the artifact via
26+
# `workflow_run`.
27+
#
28+
# Why split it?
29+
# "The GITHUB_TOKEN has read-only permissions in pull requests from forked
30+
# repositories."
31+
# — https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request
32+
# A read-only token cannot post comments, so on fork PRs the previous
33+
# single-workflow design failed with HTTP 403. We can't simply broaden the
34+
# trigger here either: cargo-semver-checks compiles PR code (build.rs, proc
35+
# macros), so granting this job a write token would expose it to any code
36+
# in the PR. And ASF infra policy independently forbids `pull_request_target`
37+
# for any workflow that exposes GITHUB_TOKEN
38+
# (https://infra.apache.org/github-actions-policy.html). The companion
39+
# `workflow_run` workflow runs in the base-repo context with write access
40+
# and never executes PR code.
2541

2642
name: "Detect breaking changes"
2743

@@ -37,11 +53,6 @@ jobs:
3753
check-semver:
3854
name: Check semver
3955
runs-on: ubuntu-latest
40-
outputs:
41-
logs: ${{ steps.check_semver.outputs.logs }}
42-
# Default to "success" so the comment job clears any stale comment
43-
# when the check step is skipped (e.g. no published crates changed).
44-
result: ${{ steps.check_semver.outputs.result || 'success' }}
4556
steps:
4657
- name: Checkout
4758
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
@@ -85,11 +96,6 @@ jobs:
8596
ci/scripts/changed_crates.sh semver-check "origin/${BASE_REF}" $PACKAGES \
8697
2>&1 | tee /tmp/semver-output.txt
8798
EXIT_CODE=${PIPESTATUS[0]}
88-
{
89-
echo "logs<<EOF"
90-
sed 's/\x1b\[[0-9;]*m//g' /tmp/semver-output.txt
91-
echo "EOF"
92-
} >> "$GITHUB_OUTPUT"
9399
# Pass the result through an output instead of failing the job:
94100
# a detected breaking change should surface as a PR comment, not a
95101
# red check, so PR authors aren't confused by an intentional break.
@@ -99,28 +105,29 @@ jobs:
99105
echo "result=failure" >> "$GITHUB_OUTPUT"
100106
fi
101107
102-
# Post or remove a sticky comment on the PR based on the semver check result.
103-
comment-on-pr:
104-
name: Comment on pull request
105-
runs-on: ubuntu-latest
106-
needs: check-semver
107-
if: always()
108-
permissions:
109-
contents: read
110-
pull-requests: write
111-
steps:
112-
- name: Checkout
113-
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
114-
with:
115-
sparse-checkout: ci/scripts
116-
117-
- name: Update PR comment
108+
# Stage the data the companion comment workflow needs into a single
109+
# directory. We default the result to "success" so the comment
110+
# workflow clears any stale comment when the check step is skipped
111+
# (e.g. no published crates changed).
112+
- name: Stage artifact for comment workflow
113+
if: always()
118114
env:
119-
GH_TOKEN: ${{ github.token }}
120-
REPO: ${{ github.repository }}
121115
PR_NUMBER: ${{ github.event.pull_request.number }}
122-
CHECK_RESULT: ${{ needs.check-semver.outputs.result }}
123-
SEMVER_LOGS: ${{ needs.check-semver.outputs.logs }}
116+
CHECK_RESULT: ${{ steps.check_semver.outputs.result || 'success' }}
124117
run: |
125-
ci/scripts/changed_crates.sh comment \
126-
"$REPO" "$PR_NUMBER" "$CHECK_RESULT" "$SEMVER_LOGS"
118+
mkdir -p semver-artifact
119+
echo "$PR_NUMBER" > semver-artifact/pr_number
120+
echo "$CHECK_RESULT" > semver-artifact/result
121+
if [ -f /tmp/semver-output.txt ]; then
122+
sed 's/\x1b\[[0-9;]*m//g' /tmp/semver-output.txt > semver-artifact/logs
123+
else
124+
: > semver-artifact/logs
125+
fi
126+
127+
- name: Upload artifact
128+
if: always()
129+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
130+
with:
131+
name: semver-check-result
132+
path: semver-artifact/
133+
retention-days: 1

‎.github/workflows/breaking_changes_detector_comment.yml‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
18+
# Companion to `breaking_changes_detector.yml` — posts the sticky PR comment.
19+
#
20+
# Why this workflow exists:
21+
# "The GITHUB_TOKEN has read-only permissions in pull requests from forked
22+
# repositories."
23+
# — https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request
24+
# That is why the upstream `pull_request` workflow cannot post the comment
25+
# itself when the PR comes from a fork.
26+
#
27+
# Why not `pull_request_target`? ASF infra policy forbids it:
28+
# "You MUST NOT use `pull_request_target` as a trigger on ANY action that
29+
# exports ANY confidential credentials or tokens such as GITHUB_TOKEN or
30+
# NPM_TOKEN."
31+
# — https://infra.apache.org/github-actions-policy.html
32+
# `workflow_run` is the supported alternative: it runs in the base
33+
# repository's context regardless of where the upstream run was triggered
34+
# from, so the GITHUB_TOKEN here can be granted `pull-requests: write`. See:
35+
# https://docs.github.com/en/actions/reference/events-that-trigger-workflows#workflow_run
36+
#
37+
# Security note: this workflow MUST NOT check out or execute any code from
38+
# the PR. The artifact's contents originate from a workflow run that may
39+
# have compiled fork-controlled code, so PR_NUMBER and CHECK_RESULT are
40+
# validated against strict patterns before being passed to any action.
41+
42+
name: "Detect breaking changes - Comment"
43+
44+
on:
45+
workflow_run:
46+
workflows: ["Detect breaking changes"]
47+
types:
48+
- completed
49+
50+
permissions:
51+
contents: read
52+
53+
jobs:
54+
comment-on-pr:
55+
name: Comment on pull request
56+
if: github.event.workflow_run.event == 'pull_request'
57+
runs-on: ubuntu-latest
58+
# Scoped to the minimum needed to upsert/delete the sticky comment.
59+
permissions:
60+
pull-requests: write
61+
steps:
62+
- name: Download semver-check artifact
63+
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
64+
with:
65+
name: semver-check-result
66+
run-id: ${{ github.event.workflow_run.id }}
67+
github-token: ${{ github.token }}
68+
path: ./semver-artifact
69+
70+
- name: Read and validate artifact
71+
id: read
72+
run: |
73+
set -euo pipefail
74+
# Validate every field: the artifact comes from a workflow run
75+
# that compiled fork-controlled code, so its contents are untrusted.
76+
PR_NUMBER=$(cat ./semver-artifact/pr_number)
77+
if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then
78+
echo "Invalid PR number: $PR_NUMBER" >&2
79+
exit 1
80+
fi
81+
CHECK_RESULT=$(cat ./semver-artifact/result)
82+
if [[ "$CHECK_RESULT" != "success" && "$CHECK_RESULT" != "failure" ]]; then
83+
echo "Invalid check result: $CHECK_RESULT" >&2
84+
exit 1
85+
fi
86+
echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT"
87+
echo "result=$CHECK_RESULT" >> "$GITHUB_OUTPUT"
88+
89+
# Multi-line output: random delimiter so a malicious log line can't
90+
# close the heredoc and inject extra output keys. See:
91+
# https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#multiline-strings
92+
DELIM="EOF_$(openssl rand -hex 16)"
93+
{
94+
echo "logs<<${DELIM}"
95+
cat ./semver-artifact/logs
96+
echo "${DELIM}"
97+
} >> "$GITHUB_OUTPUT"
98+
99+
# The marker `<!-- semver-check-comment -->` is what makes the comment
100+
# "sticky": maintain-one-comment uses it to find and replace (or
101+
# delete) the existing comment instead of stacking new ones.
102+
- name: Upsert sticky comment
103+
if: steps.read.outputs.result != 'success'
104+
uses: actions-cool/maintain-one-comment@909842216bc8e8658364c572ec52100f4c2cc50a # v3.3.0
105+
with:
106+
token: ${{ secrets.GITHUB_TOKEN }}
107+
number: ${{ steps.read.outputs.pr_number }}
108+
body-include: '<!-- semver-check-comment -->'
109+
body: |
110+
<!-- semver-check-comment -->
111+
Thank you for opening this pull request!
112+
113+
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
114+
115+
<details>
116+
<summary>Details</summary>
117+
118+
```
119+
${{ steps.read.outputs.logs }}
120+
```
121+
122+
</details>
123+
124+
- name: Delete sticky comment
125+
if: steps.read.outputs.result == 'success'
126+
uses: actions-cool/maintain-one-comment@909842216bc8e8658364c572ec52100f4c2cc50a # v3.3.0
127+
with:
128+
token: ${{ secrets.GITHUB_TOKEN }}
129+
number: ${{ steps.read.outputs.pr_number }}
130+
body-include: '<!-- semver-check-comment -->'
131+
delete: true

‎ci/scripts/changed_crates.sh‎

Lines changed: 1 addition & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,9 @@
2828
# Run cargo-semver-checks for the given packages against base_ref.
2929
# Output and exit code are passed through unchanged; the caller is
3030
# responsible for capturing/formatting them.
31-
#
32-
# comment <repo> <pr_number> <check_result> [logs]
33-
# Upsert or delete a sticky PR comment based on check_result.
34-
# check_result: "success" deletes any existing comment,
35-
# anything else upserts the comment with the provided logs.
36-
# Requires GH_TOKEN to be set.
3731

3832
set -euo pipefail
3933

40-
MARKER="<!-- semver-check-comment -->"
41-
4234
# ── changed-crates ──────────────────────────────────────────────────
4335
cmd_changed_crates() {
4436
local base_ref="${1:?Usage: changed_crates.sh changed-crates <base_ref>}"
@@ -80,62 +72,12 @@ cmd_semver_check() {
8072
cargo semver-checks --baseline-rev "$base_ref" "${args[@]}"
8173
}
8274

83-
# ── comment ─────────────────────────────────────────────────────────
84-
cmd_comment() {
85-
local repo="${1:?Usage: changed_crates.sh comment <repo> <pr_number> <check_result> [logs]}"
86-
local pr_number="${2:?}"
87-
local check_result="${3:?}"
88-
local logs="${4:-}"
89-
90-
# Find existing comment with our marker
91-
local comment_id
92-
comment_id=$(gh api "repos/${repo}/issues/${pr_number}/comments" \
93-
--jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" | head -1)
94-
95-
echo "existing breaking change comment id $comment_id"
96-
97-
if [ "$check_result" = "success" ]; then
98-
# Delete the comment if one exists
99-
if [ -n "$comment_id" ]; then
100-
echo "result is success, so deleting breaking change comment"
101-
gh api "repos/${repo}/issues/comments/${comment_id}" --method DELETE
102-
else
103-
echo "result is success and no previous comment to delete"
104-
fi
105-
else
106-
local body="${MARKER}
107-
Thank you for opening this pull request!
108-
109-
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
110-
111-
<details>
112-
<summary>Details</summary>
113-
114-
\`\`\`
115-
${logs}
116-
\`\`\`
117-
118-
</details>"
119-
120-
if [ -n "$comment_id" ]; then
121-
echo "comment already exists, updating content"
122-
gh api "repos/${repo}/issues/comments/${comment_id}" \
123-
--method PATCH --field body="$body"
124-
else
125-
echo "no comment with breaking changes, creating a new one"
126-
gh api "repos/${repo}/issues/${pr_number}/comments" \
127-
--method POST --field body="$body"
128-
fi
129-
fi
130-
}
131-
13275
# ── main ────────────────────────────────────────────────────────────
133-
cmd="${1:?Usage: changed_crates.sh <changed-crates|semver-check|comment> [args...]}"
76+
cmd="${1:?Usage: changed_crates.sh <changed-crates|semver-check> [args...]}"
13477
shift
13578

13679
case "$cmd" in
13780
changed-crates) cmd_changed_crates "$@" ;;
13881
semver-check) cmd_semver_check "$@" ;;
139-
comment) cmd_comment "$@" ;;
14082
*) echo "Unknown command: $cmd" >&2; exit 1 ;;
14183
esac

0 commit comments

Comments
 (0)