Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pickSHA: Improve pickSHA to handle pull_request_target and push #365

Merged
merged 1 commit into from
Oct 27, 2024

Conversation

bjhargrave
Copy link
Contributor

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
Copy link

@dghubble could you PTAL?

@dghubble
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Thank you BJ, that all seems reasonable to me

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."

Signed-off-by: BJ Hargrave <[email protected]>
@bjhargrave
Copy link
Contributor Author

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

@dghubble dghubble merged commit f177115 into poseidon:main Oct 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants