Skip to content

build: allow posting comments on PRs made from forks and fix missing protobuf#21913

Merged
rluvaton merged 5 commits intoapache:mainfrom
rluvaton:fix-breaking-changes-detector-fork-prs
Apr 30, 2026
Merged

build: allow posting comments on PRs made from forks and fix missing protobuf#21913
rluvaton merged 5 commits intoapache:mainfrom
rluvaton:fix-breaking-changes-detector-fork-prs

Conversation

@rluvaton
Copy link
Copy Markdown
Member

@rluvaton rluvaton commented Apr 29, 2026

Which issue does this PR close?

Rationale for this change

The breaking-change detector added in #21499 fails on fork PRs with HTTP 403:

The GITHUB_TOKEN has read-only permissions in pull requests from forked repositories.

From GitHub Docs

A read-only token can't post the sticky comment, so the workflow errors out at the gh api … POST /comments call.

We can't switch to pull_request_target either - ASF infra policy forbids it for any workflow exposing GITHUB_TOKEN (https://infra.apache.org/github-actions-policy.html), and cargo-semver-checks compiles fork-controlled code (build.rs, proc macros) anyway, so granting it a write token would be unsafe.

What changes are included in this PR?

Split the comment posting into a companion workflow_run workflow:

  • breaking_changes_detector.yml keeps the pull_request trigger but only stages the result (pr_number, result, logs) and uploads it as an artifact. No write token, no comment posting from this workflow.
  • breaking_changes_detector_comment.yml triggers on workflow_run, runs in the base-repo context with pull-requests: write, downloads the artifact, validates the inputs, and upserts/deletes the sticky comment via actions-cool/maintain-one-comment. Never checks out PR code.

The comment workflow uses a runtime-randomized heredoc delimiter when piping the fork-controlled logs into $GITHUB_OUTPUT, to stop log content from closing the heredoc early and overwriting the validated result output (or injecting other keys).

Drops the now-unused comment subcommand from ci/scripts/changed_crates.sh.


also installed protobuf as noticed this failed when building subtrait in:

Are these changes tested?

No, cant really test it

Are there any user-facing changes?

No

@github-actions github-actions Bot added the development-process Related to development process of DataFusion label Apr 29, 2026
# delete) the existing comment instead of stacking new ones.
- name: Upsert sticky comment
if: steps.read.outputs.result != 'success'
uses: actions-cool/maintain-one-comment@909842216bc8e8658364c572ec52100f4c2cc50a # v3.3.0
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rluvaton rluvaton changed the title build: allow posting comments on forked PRs build: allow posting comments on forked PRs and fix missing protobuf Apr 29, 2026
@rluvaton
Copy link
Copy Markdown
Member Author

@Jefffrey Can you please review?

@rluvaton rluvaton requested a review from Jefffrey April 29, 2026 09:13
@rluvaton rluvaton changed the title build: allow posting comments on forked PRs and fix missing protobuf build: allow posting comments on PRs made from forks and fix missing protobuf Apr 29, 2026
@Jefffrey
Copy link
Copy Markdown
Contributor

I'm not so sure about this level of complexity now, with needing to communicate between two workflows via uploading an artifact 🤔

Maybe we should revert the previous PR in the meantime while we get more eyes on this

Copy link
Copy Markdown
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread .github/workflows/breaking_changes_detector_comment.yml
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@rluvaton rluvaton enabled auto-merge April 30, 2026 06:53
@rluvaton rluvaton added this pull request to the merge queue Apr 30, 2026
Merged via the queue into apache:main with commit 0bb17bc Apr 30, 2026
35 checks passed
@rluvaton rluvaton deleted the fix-breaking-changes-detector-fork-prs branch April 30, 2026 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of DataFusion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants