-
Notifications
You must be signed in to change notification settings - Fork 43
Explicitly checkout to PR commit #565
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
Conversation
| - name: Checkout to the correct commit | ||
| working-directory: ${{ inputs.package }} | ||
| run: | | ||
| git fetch https://github.com/${{ inputs.repo_owner }}/${{ inputs.package }}.git refs/pull/${{ inputs.pr_number }}/merge:refs/remotes/pull/${{ inputs.pr_number }}/merge | ||
| git checkout refs/remotes/pull/${{ inputs.pr_number }}/merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we checkout to the merge commit (this is what we get if the event is pull_request without the change in this PR, so the behavior would be consistent in both event types)
gante
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
question: wouldn't ref: ${{ inputs.commit_sha }} on L81 work as well?
|
(tagged @hanouticelina for a second pair of 👀 ) |
|
You are likely right. The nit is: in the caller (the workflow I created in .github/workflows/pr_build_doc_with_comment.ymlwhich is the head sha of the branch, and that corresponds to .github/workflows/build_pr_documentation.ymlthat also use the same head sha. However, arriving If we use However, currently (without this PR), it will checkout to the so called So we want to:
And if we don't grab this merge commit sha from the caller (which is extra work), the step I added is necessary |
hanouticelina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually approving ✅
my understanding of the PR: the extra step makes sure that no matter how the doc build is triggered, it always checks out the correct PR merge. That makes sense to me!
|
@ydshieh thank you for the detailed explanation 💛 |
Allow passing commit sha of the package, so we can checkout to it.
This is necessary when we want to trigger doc-build on a PR with other trigger event, like leaving a comment
build-doc, otherwise it will checkout to themainbranch in this case