PR Checkout bug or misunderstanding?

[chanan]

Hello,

We have been using actions with checkout for a while now, and we just noticed that either we weren't using it right or there is a bug in the latest v2. We run a workflow on every commit to a PR against master like the example here: https://github.com/actions/checkout#checkout-pull-request-on-closed-event

We created a repo to show the issue: https://github.com/Marketron/CheckoutTest

Sequence of events (can be seen in the commit log):

1. Created a file in master: MyFirstCheckInToMaster.md
2. Create a workflow in a PR 1 called pr.yml the workflow is simple, on any commit to a PR (against master) it will checkout the PR and run ls -al https://github.com/Marketron/CheckoutTest/blob/master/.github/workflows/pr.yml. The action seems to only checkout the PR code, as expected, as seen in the action log: /usr/bin/git checkout --progress --force refs/remotes/pull/1/merge
3. Merge that PR to master
4. Add another file to master called: SecondMasterCheck.md
5. Create a branch from the point in time where I merged the first PR (step 3 above)
6. Create a file in that branch called: CheckIntoTestPrBranch.md
7. Created a PR 2 and observed the ls -al command in it: https://github.com/Marketron/CheckoutTest/runs/4023463428?check_suite_focus=true#step:3:11

Expected result:
Should only list two files: MyFirstCheckInTiMaster.md & CheckIntoTestPrBranch.md

Actual result:
Lists 3 files including SecondMasterCheck.md as seen here: https://github.com/Marketron/CheckoutTest/runs/4023463428?check_suite_focus=true#step:3:11

So is this a bug or have we been wrong on how PR checkout works or the params we need to send to actions/checkout@v2?

[chanan]

Ok, so it seems like to fix this case adding: ref: ${{ github.event.pull_request.head.sha }} fixes the above problem.

I am still confused then when that is needed. According to the docs that shouldn't be needed on a PR: https://github.com/actions/checkout#checkout-pull-request-on-closed-event

[polarathene]

It is confusing to experience but probably the correct behaviour desired for CI usually. See this related issue.

You can still perform a diff with git on the default merge commit to identify what files in the history the PR is actually contributing, that would identify CheckIntoTestPrBranch.md like the PR diff view on Github would show.

---

The reason for preferring the newer history being present, is to ensure that any functionality being added doesn't break when merged to the base branch (eg: master).

Although the PR itself seemed fine and any tests run were successful, after merging the tests may fail on future PRs (or a release/build of master on each push to that branch which may likewise be broken) even though it is not responsible for introducing the breakage (which would be more confusing). All due to the earlier PR not testing against newer changes on the base branch to ensure compatibility.

That can occur due to another change on the base branch (be that another merged PR or direct push like your SecondMasterCheck.md) that has overlapping functionality change with your 2nd PR as this page explains.

---

When that is not a concern for your PR workflow, then you can ignore the default ref, and choose the head.sha context (which is more reliable than the head.ref which is not as deterministic for job re-runs). Then you'll have the PR branch only, and not the result of merge to the base branch.

In some cases such as linting changes from the PR, you may prefer that. But you could still identify (from the merge commit with a fetch-depth: 2) the files changed by the PR, and lint those (with the context of being merged into master) where those files may have received other changes worth catching within the context of the PR changes.

An example would be for docs: Adding a relative link to another document or a non-perma link on your github repo which has since been moved (or renamed due to some accidental typo in the filename). Without the default merge commit, your workflow on the PR would not catch that issue, and your checks would pass, but then it's broken again once the PR is merged.