c p r bot clobbers commits to PR when run as cron job

[jamesp]

c p r bot clobbers commits to PR when run as cron job

Developer pushes to the create-pull-request branch are being clobbered by create-pull-request running as a cron job.

We have a GHA workflow that updates python dependencies and saves them as a lockfile that is then turned into a PR by create-pull-request. The workflow runs once a week on a Saturday and pushes to branch auto-update-lockfiles. Commits that we push to the same branch are clobbered the following Saturday by the bot.

I assume this is because CPR, when run as a cron job, is using master:HEAD as the base for the starting point. There may be an easy and obvious way around this, advice would be welcome. I think I want something like this perhaps, where base is the auto-update-lockfiles branch if it exists, or master if not. Is this possible?

      - name: Create Pull Request
        uses: peter-evans/create-pull-request@052fc72b4198ba9fbc81b818c6e1859f747d49a8
        with:
          ...
        branch: auto-update-lockfiles
        base: auto-update-lockfiles || master

Steps to reproduce

See the actions workflow here:
https://github.com/SciTools/iris/blob/master/.github/workflows/refresh-lockfiles.yml

And resulting PR here:
SciTools/iris#4137

Note that the PR 4139 that was merged into this branch has been clobbered by the force push from the bot a few days later.

[peter-evans]

Hi @jamesp

This is expected behaviour by the action. When a workflow executes the action to create or update a PR, fundamental to the design of the action is that the result of those two paths should not be different. Rebasing the PR branch and adding new commits is the best way to maintain that consistency. I have experimented in the past with trying to keep the full commit history each time the PR branch is updated, but it's more trouble than it's worth.

In my experience, the only time I've need to make changes to an automated PR is when the automated change has caused CI to break and I want to make a small fix before merging. In this scenario the PR is merged as soon as the fix is made, so there is no opportunity for it to be clobbered. (Incidentally, I think that solutions like dependabot work similarly. If a new version of a dependency is available, dependabot closes the existing PR (clobbering any fix you might have made) and raises a new PR for the new version).

Here are some suggestions:

1. Only make changes to an automated PR if you intend to merge it straightaway.
2. If you really need to make changes but cannot merge straightaway, I would recommend creating your own PR that will merge into the automated PR just before merging. You can make your changes there and they won't be clobbered by the automated PR updates.
3. I don't really recommend this solution, but just to make sure you are aware of the option. You could use one of the alternative branching strategies to make a new PR each time the action runs. This would preserve any changes you make, but the downside is that it would make a brand new PR each time the action executes.

[jamesp]

Thanks @peter-evans for the detailed response and confirmation this is expected behaviour. I think the best thing to do for now will be for us to adjust our workflow to not leave uncommitted changes on that PR.

I was thinking about whether it might be possible for the action to abort if it detects that there have been additional pushes to the branch it wants to update. Looking at the code here (I think this is the right place!)

create-pull-request/src/create-or-update-branch.ts

Lines 200 to 215 in 9825ae6

	// Reset the branch if one of the following conditions is true.
	// - If the branch differs from the recreated temp branch.
	// - If the recreated temp branch is not ahead of the base. This means there will be
	// no pull request diff after the branch is reset. This will reset any undeleted
	// branches after merging. In particular, it catches a case where the branch was
	// squash merged but not deleted. We need to reset to make sure it doesn't appear
	// to have a diff with the base due to different commits for the same changes.
	// For changes on base this reset is equivalent to a rebase of the pull request branch.
	if (
	(await git.hasDiff([`${branch}..${tempBranch}`])) ||
	!(await isAhead(git, base, tempBranch))
	) {
	core.info(`Resetting '${branch}'`)
	// Alternatively, git switch -C branch tempBranch
	await git.checkout(branch, tempBranch)
	}

I don't think it's possible to, in the most general terms, be sure if the branch has been updated by someone else other than the bot since the last run of the action.

However, for our specific use case I will experiment with adding an additional step to the workflow to check:
IF
The auto-update-lockfiles branch exists AND the last push was NOT github bot
THEN
abort workflow.

This should prevent us from clobbering commits when we're too slow merging them!

[peter-evans]

However, for our specific use case I will experiment with adding an additional step to the workflow to check:
IF
The auto-update-lockfiles branch exists AND the last push was NOT github bot
THEN
abort workflow.

This is a good idea. 👍
If you get this additional step working please share it with me. I would be very interested to see it.

This is the first time I've seen an issue about this, but if it becomes a common problem with many users I might consider adding some kind of no-clobber feature to the action in future. For now, I think an additional workflow step is the right way to go.

[jamesp]

If you get this additional step working please share it with me. I would be very interested to see it.

A simple first pass attempted in the no_clobber job here: https://github.com/SciTools/iris/blob/cf5df97279bec7fb8201c0afcd461e43c591a37c/.github/workflows/refresh-lockfiles.yml

I'll update you if we have unforeseen issues with it. If you do have thoughts on how a robust no-clobber feature could be added, I'd be happy to contribute to developing it.