search

poseidon / wait-for-status-checks

GitHub Action that waits for check runs
https://github.com/marketplace/actions/wait-for-checks
Mozilla Public License 2.0
33 stars 13 forks source link

pickSHA: Improve pickSHA to handle pull_request_target and push #365

Closed bjhargrave closed 1 month ago

bjhargrave commented 2 months ago

To support use in pull_request_target target workflows, we need to use the pull_request.head.sha value for check-runs.

We also better support push target workflows by using the after SHA value as it better represents "The SHA of the most recent commit on ref after the push."

nathan-weinberg commented 1 month ago

@dghubble could you PTAL?

dghubble commented 1 month ago

Can you provide way more detail and context? I don't use pull_request_target or have interest in spelunking, tell me about them, your usage, the payloads you're seeing. push is already supported, tell me about the situations where you think the sha should be different.

bjhargrave commented 1 month ago

The pull_request_target target is a variation of pull_request target that runs on the context of the main (default) branch instead of the pull request branch. Such workflows can execute will full permissions of the repo since the main branch is trusted while the pull request branch is untrusted. Both targets receive the pull_request payload. Without this change, pull_request_target target workflows use the last commit to the main branch to locate check runs which are not the check runs of the PR being built.

Regarding the push payload, it documents the after parameter as the "The SHA of the most recent commit on ref after the push" which is what we want. The github.sha property is more vaguely defined as "The commit SHA that triggered the workflow. The value of this commit SHA depends on the event that triggered the workflow." I have seen some events where the github.sha property was not the desired SHA for properly locating the check runs. So for the push event, it seems better to use the event's more precisely defined after property (like we use the event pull_request.head.sha property for pull_request and pull_request_target targets).

We are currently using these changes (via my fork) in the instructlab/instructlab repo to control job ordering and dependencies. Some of the workflows using this are pull_request_target target workflows. We would prefer to use this repo over my fork repo, but need this PR to be merged and released.

Thanks.

dghubble commented 1 month ago

Thank you BJ, that all seems reasonable to me

bjhargrave commented 1 month ago

I updated the commit to rebase on HEAD and resolve the merge conflict.