SEZ9 commented on PR #12725: URL: https://github.com/apache/seatunnel/pull/12725#issuecomment-6207633486
Thanks @goutamadwant for the update and for expanding the "Behaviour differences" section — the post-merge check / rollback note is now covered there, which works well given the old `pull_request_target` workflows keep running until the cutoff. Head is still `24dc7680` with no code change since the last review, and both the fork `Build` run (https://github.com/goutamadwant/seatunnel/actions/runs/38155539260) and the apache-side check (https://github.com/apache/seatunnel/runs/114517665398) are green. As already noted on this PR, that run is workflow-only and does not exercise the new `workflow_run` consumers, so the sandbox evidence in the description remains the main proof for the trigger path. Two points raised in the earlier review on this head still need a decision, either as a small update here or as explicitly deferred follow-ups: 1. **Conclusion gate** — `success || action_required` means a `cancelled`/`failure`/`startup_failure`/`timed_out` signal run leaves the PR with no `Build` check and no labels, and nothing else can bootstrap the required check. Could you widen the gate to every terminal conclusion (the consumer only needs the PR number from the signal run), or describe how a contributor recovers in that case? 2. **Unconditional `queued` check** — every signal completion creates a fresh `queued` `Build` check for the same head SHA, so the second completion (`action_required` → approved) or a committer re-run flips an already-green required check back to pending. Looking up existing check runs for the head SHA and skipping creation when one is already `completed`/`success` would address this. The remaining Low items are at your discretion, but quick confirmations would help: is the contributor guide updated for the no-`Build`-check-on-conflict case; is the "only reads API metadata" comment corrected to reflect the label/check writes; and is the stale `pull_request.user` `TODO` removed? For the labeler config now being read from the default branch and the coupling to the `PR signal` display name, a sentence in the description acknowledging the behaviour is enough if you decide not to change them. None of these are blockers; a committer approval is still required for merge. <!-- streview-comment:1801 --> -- 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]
