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
What does "Checkout pull request HEAD commit instead of merge commit" mean? #426
Comments
The docs below may help, let me know https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/merging-a-pull-request By default a merge commit is checked out. If you look at the SHA that is checked out, it doesn't match your local commit SHA. Rather it's the SHA of a merge commit (your branch + base branch) |
Thanks for the response!
|
Would there be any interest in revisiting this decision at all? |
To further clarify: the merge commit is checked out for
The default action for merging of a Pull Request is a merge commit, so (IMO) I think it is the appropriate default for the checkout action to run against what the pr "merge" button will generate (by default). For the historical github flow (before squash and rebase merges were available on github), it would be more surprising (IMO) to have the HEAD run CI, since that doesn't provide any safety or guarantees which are the purpose of the CI jobs. Imagine a green CI check that fails immediately upon merge; that's a much more dangerous scenario since it actively promoting false negatives.
Agreed.
FWIW, this is also the default behavior for travisci. (actually, I believe the default for travis is to run two builds: one for push and one for pr; but the pr build is built against the merge commit) |
I just spent 4 hours scratching my head trying to understand why I got different sha for I really wish this information would be more visible and the implications of it (especially for non-experts in git/github). I naively thought that running |
Just want to highlight this is mentioned in the docs now:
|
So that's actually exactly what I just bumped into with the current behaviour. Our repository has submodules and we have some bots that tell us when these need to be updated. In our main branch these have been moving forward so the main branch is a couple of versions ahead on some of these submodules. In the mean time, we're preparing a major release on a separate version 4.0.0 branch. We've got lots of smaller PRs being merged into that v4 branch. For one of those PRs, the tests were all green and so we merged it into the v4 branch. As soon as we did that, things failed in the v4 branch (same code and tests that just passed in the PR). The only difference between the two runs in CI was the merge target... first time round the merge target was the v4 branch (which didn't have any of the submodule updates applied). Second time round, the merge target was our main branch (which did have submodule updates applied)... so different results. Specifically (and this is maybe too much detail) in our case the submodules are stuff running in other languages. As part of our build process, we generate some wrappers to do marshalling etc. so that we can make use of native code in those modules and we check to make sure that that generated code is the same, after build, as what was checked into source control. If any of the files are dirty, CI fails as it knows something funky has happened. If we instead checked out the PR head, I think the only risk would be if some code in the PR relied on generated/marshalling code that had undergone a breaking change - but that would imply a major version bump for one of the submodules we reply on... So at least in our specific case, it seems like we'd be better off using |
FWIW, since my earlier post I've reversed my opinion -- I've seen proprietary data which has led me to believe that checking out the merge commit, while not as pure as always checking out the PR head, results in better outcomes overall. |
@sunshowers I'd be really interested to hear specific examples of how/where |
It's not a problem per se, it's just that the data showed that using the merge conmit provided more relevant CI results. |
of course it can, for example in our CI we must run tests on content of feature branch of the PR merged with content of the dev trunk. if we run on feature branch content only, it will pass, but the moment it gets merged to trunk and code is mixed among them, it'll fail. I am actually wondering how running a CI on incoming branch only, can be useful as a quality gate for merge (i mean, after all we're usually validating that after the merge to trunk or some 'main', the code doesn't break the 'main' right? the tests run and all that. but if we don't run it on a merge commit where code is joined of both source and destination, those tests are invalid and won't protect the main branch from breaking imho). I hope I understood the discussion correctly =) . |
but anyways, I came here while breaking my head searching for a solution of how to checkout the merge commit from pre-existing repo that I am caching in the ec2 AMI in order to not checkout many GBs of LFS and 180k+ files monorepo each time (so not doing a fresh checkout of PR during workflow, but manual 'git checkout' in cli, to the $GITHUB_SHA commit, which is the merge commit in |
hahaaa i figured this out 🎉
the fetch step with correct format of |
This is a powerful reason for the current behaviour, Nonetheless, it is important to bear in mind that there is a substantial distinction between a "green" checkmark in the most recent pull request and one from a week ago. Ideally, I hope that GitHub could integrate this awareness into the icon to clearly convey this difference. |
What if base branch is updated after pull_request workflow checks out merge commit and tests are running, PR workflow will succeed but I think these tests will also be invalid. I don't see options other then to also run tests on push event on the base branch. |
#2095 switched our CI provider from CircleCI to GitHub Actions, but broke our integration with CDash. The checkout action provided by GitHub runs workflows triggered by the `pull_request` event against a [merge commit](actions/checkout#426) instead of the PR's head ref. This causes issues with the CDash integration since the git hash CDash receives does not match the most recent hash on the pull request. I was not able to resolve the callback issue, but the changes in this PR improve the CDash build name being submitted, and assign runs to appropriate build groups. I plan to take another look at this issue at a future point to restore our CDash callback.
Edit 2023-10-23: I no longer believe in what I wrote below -- I've seen data which has changed my belief into checking out the merge commit is the right default. I'll leave this open since there's a lot of passion here, and maybe this deserves clearer documentation.
Hi there!
The README has a section saying how to "[c]heckout pull request HEAD commit instead of merge commit". It isn't really clear what this means, and I tried looking at the source code and it didn't really make sense to me either.
Could you add some more information about this to the README? What is a "merge commit" in this situation?
Some information that might be useful:
ref
set to${{ github.event.pull_request.head.sha }}
My experience with CI has led me to believe that there's just one way to check out a pull request (the most recent commit on the branch at the time the PR was initiated or last updated) so when I came across this option I was really surprised.
The text was updated successfully, but these errors were encountered: