Only run checkout if not running locally with act, and update to latest merge ref #61
Conversation
When a GitHub action is re-run, it uses the same context as the original run. This means that it runs on the exact same merge commit and does not pick up changes done to the base branch since the last run.
In case the base branch is was failing when a PR was opened, this means that we can’t get CI to pass by fixing the base branch without pushing a new commit to the PR branch.
Adding `ref: ${{ github.ref }}` to the `actions/checkout` action works around this by checking out the PR merge ref (ie. `refs/pull/<pr_number>/merge`) instead of a fixed commit and the PR merge ref is updated, as described in actions/checkout#1036.
|
//cc @ahoppen @shahmishal @rnro |
|
@simonjbeaumont does running locally work with and without |
No, now you must use |
|
Converting this to a draft PR because there's some discussion going on about whether we want this or not. |
There was a problem hiding this comment.
I have been thinking about this PR and the original PR from you @ahoppen a bit more and in general I think this is the wrong thing to do. If I understand the intention correctly here the problem that we were facing was that when re-running actions that it took exactly the same commit even if the target branch has been updated.
This is IMO expected. The reason why GitHub Actions work this way is that they want to offer you a reproducible environment when re-running. Especially when re-running only failed checks they want to make sure they are as close to the other checks as possible. With the change in #58 this was no longer the case and if the target got updated any re-run included those changes. IMO this is dangerous and I would heavily advocate for not doing this. What can happen is that some checks pass and some fail, then the target branch gets updated, only the failed checks get re-run and it looks like everything is green when in fact some of the previously passed checks would be failing now.
This trade off does not seem worth it when the work around is as easy as updating your branch with the target branch.
In fact in a lot of our server repositories, we mitigate this by requiring branches to be up-to-date before merging to make sure the checks from the PRs are as up-to-date as possible. If this becomes a problem when merging many PRs at the same time GitHub offers merge queues as a solution around that.
@ahoppen Let me know what you think about this.
|
Maybe for some background on why I want this: swiftlang/swift-format#712 is a PR by an external contributor that had a CI failure on Windows after we released the Windows 6.0.2 toolchain. I addressed that failure in swiftlang/swift-format#869 but I can’t get his PR green unless I ask him to rebase his PR (or I rebase the branch on his fork, which I generally consider rude). Having a the update branch button on GitHub falls into a similar category as me rebasing their branch. But I don’t think that contributors should be required to take action because there was an issue on our side. Generally Similarly, consider that there is an open PR where you are unsure whether it might be affected by recent upstream changes but where I am not sure. I think it’s very valuable if I can re-run CI with a new base branch, again without having to rebase your PR branch. |
|
How did you make sure that your change on the base branch didn't break a CI pipeline that passed earlier, if you didn't rerun it on the new merge commit? |
Sorry, I don’t understand this question. Which change, CI pipeline and merge commit are you referring to? |
|
If you have a PR whose head is at commit A, the main branch is at commit M1, and your CI first runs on the merge commit A+M1. One job passes (J1) and one fails (J2). You then commit M2 to the main branch, and restart only the failing job (J2), which succeeds on the merge commit A+M2. How do you make sure that job J1 isn't now broken on A+M2? You never ran in on that merge commit, yet your PR is now fully green. But each CI job ran on a different state of the repo. |
I understand the problem that you are facing but IMO this is really the wrong solution. As pointed out in my comment and @czechboy0 comment this change would effectively allow a PR with green checks to be merged that in fact wouldn't pass if re-run against the latest merge commit. However, GitHub offers two solutions to your problem here:
Both of those enable you as the repo owner to take an action to update a PR. The former, adds a button to a PR to update the branch. (I think it requires the PR creator to allow edits but if they don't allow this then it really is up to them). The latter allows you to enqueue a PR into a queue of merges. This queue will make sure that it runs against the latest head of the target branch. |
|
I don’t think that re-running the tests on a merge commit that merges your PR to whichever commit was the head of the target branch when you opened your PR is really meaningful. As soon as commits come into the target branch, re-running testing on that specific merge commit is not meaningful because it won’t be a state that will ever be reached by merging the PR.
I don’t think this problem is unique to re-running a single job. You can get into the same situation by testing your entire PR, then you merge a change to the target branch, which is incompatible with the PR but the GitHub Action status is still green. I am approaching this from the perspective of Swift CI on the compiler repo, where changes come in at a pretty rapid stream and PR testing takes 3-4h, so it’s not feasible to require your PR branch to be up-to-date with the target branch. And finally, all the Swift packages that are part of the toolchain use branch-based dependencies. Thus re-running the GitHub action pulls in the latest changes from all those branch-based dependencies but not from the repo itself. This just seems odd. If you feel very strongly about this, we could make this behavior an option in the workflow. |
FWIW I feel very strongly that this is the wrong solution to the problem. The CI ecosystem has acknowledged that problem many years ago. It is also acknowledged that it is really problematic that the checks on a PR are almost irrelevant if you don't test against the latest head i.e. against the latest merge commit. That's why GitHub introduced the update branch setting. However, you are completely right that this is infeasible to turn on in larger repositories since it becomes a race to merge. I would even argue that in repos with high traffic having the potential of breaking The industry accepted solution for this are merge queues. Good articles about them are:
The good thing is merge queues are offered by GitHub and can easily be turned on. I think the Swift repo might hugely benefit from such a merge queue setup and it shouldn't be too complicated to get setup https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue. @shahmishal Would be interested to hear what you think about this! |
|
Just to add to this. I fully understand where you are coming from w.r.t. the Swift repo and if adding this unblocks you I am fine with adding it as an opt-in feature but I would really encourage you to look at the potential of merge queues. IMO it would positively impact the stability of the |
|
I think a major limitation of merge queues is that it wouldn’t be possible to do land changes that need to modify multiple repos at the same time (at the moment we get two linked PRs into a green CI state and then hit merge on both PRs at the same time). Such changes are somewhat frequent, eg. a swift change that needs a corresponding llvm-project change or a swift-syntax change that needs an accompanying swift-format or swift change. |
I don't think this is necessarily true. We can probably come up with a solution here where we parse the commit messages to check if there are linked PRs and then make sure to update the checkout of the dependencies to that. It might be that such PRs can't be queued up with other PRs that also need dependencies. You certainly can come up with a model. |
|
Not exactly what would be fixed by this PR but related because it’s also caused by not updating a reference when re-triggering a run: #67 broke testing for packages on Windows, which caused https://github.com/swiftlang/swift-format/actions/runs/12159769017/attempts/2?pr=883 to fail. Now, the issue in the github-workflows repository was fixed by #71 but re-running the CI testing still uses the old checkout of github-workflows, not picking up the fix. The only way to work around this is that the contributor needs to make a non-sensical change to their PR, which allows us to re-run testing with an updated github-workflows. |
I understand the potential problems that this can cause together with the way that GitHub has implemented the re-run functionality. When re-running they are making sure to use exactly the same workflow definition and commits so that the results are the same. While I agree that this can cause confusing behaviour and I personally think that they should at least pick up workflow file changes. This is out of our control. Even checking out the latest merge ref would not have fixed this from what I can see. |
|
We just had a similar issue again where I needed to ask a contributor to rebase their changes to re-trigger CI: swiftlang/swift-format#950 As far as I can tell the issue here is that GitHub updated their Windows runners, which causes compilation errors. I was able to work around that by starting to run tests inside a Docker container again on Windows (swiftlang/swift-format#951) but the contributor now needs to rebase and push their changes to pick up that fix. |
|
If I recall our offline discussion correctly we said that it is okay to land this back in and repositories that want to enforce up-to-date PRs can do this by using the appropriate GH setting or by adopting merge queues to ensure linear PR merges. So from my side we can unblock this PR again since @simonjbeaumont fixed the original problem that fixed the destructive local changes. |
Can't you just click the button for them to "Update branch" which will merge main into their branch. This shouldn't require contributor interaction. If the update branch fails, then the merge would be failing anyhow since there would be conflicts that need resolving and CI wouldn't run in any event.
What's the action here—does this PR need rebasing and merging? I'll wait for confirmation before actually doing it as there was quite some discussion and I'd rather have it all settled and not cause an issue by jumping the gun. |
Motivation
#58 added configuration to the
actions/checkoutstep to update to the latest GitHub merge ref. This meant that rerunning a job, would also re-evaluate the merge ref, if the target branch had moved on.It was reverted in #60 because it had some unintended and destructive consequences when run locally with
act.However, we'd like to restore that behaviour when running in CI.
Modifications
actions/checkout@v4step in each job if not running withact.Result
act, no checkout step is run at all.