Use merge commit for PR instead of HEAD commit of PR

[felixfontein]

Fixes #3. See ansible-collections/community.docker#280 as an example; it was run with ansible-collections/community.docker@c4a009a.

[felixfontein]

ansible-collections/community.docker#280 shows that this works. (After moving the workflow changing comment to main and adding another comment, it worked!)

[felixfontein]

Bah, this is really a huge mess. For whatever reason, it never uses the correct commit, until I force it down its throat. But the ref I'm using is the merge commit of the HEAD of the PR's branch, which is wrong if you re-run the action for an older commit of the PR.

[felixfontein]

See https://github.com/ansible-collections/community.docker/actions/runs/1674072333 for a re-run with the version of this branch at this state (61cdf7c).

[briantist]

This looks good, thank you @felixfontein ! Using the merge commit seems like the better option, even if it means re-runs of old commits are not accurate.

If we wanted to go further with it, we could probably introduce calls that check if the current run is against the PR's latest commit or not, to detect a re-run of an old commit (we would probably use the github-script action and its build in support for making API calls, since the run itself will have the old information in the event).

What we'd do with that information then, is possibly:

• skip the process?
• change which checkout we're doing (which means we might even get a different result than when we ran originally with the merge commit)
• emit a warning in the run's output about the situation

Ultimately that's probably overkill; providing an option for callers to use the merge commit vs. head sha is probably also overkill.

So I think this is great for now, and we'll consider anything further later, if the need arises.

[felixfontein]

@briantist thanks!

[felixfontein]

Hmm, one drawback of this seems to be that when run after closing the PR, building the docs fails: https://github.com/ansible-collections/community.docker/runs/4757572332?check_suite_focus=true

Maybe we have to skip this part when run on PR close? (Or maybe I also deleted the branch too fast?)

[felixfontein]

I've tested this with a merged PR, same behavior: https://github.com/ansible-collections/community.docker/runs/4757900203?check_suite_focus=true

[briantist]

Thanks for testing that, this does leave us in a tricky position.

From a client POV (like from community.docker), if you are not interested in customized messages on issue close, then the PR comment action can be told instead to just remove any existing comment, by setting these parameters to remove and not setting the on-*-body.

In that case you might also add a conditional to the call to the build so that it doesn't run at all on closed (it's only needed on close if you want to know whether there were changes, so that the message can be chosen accordingly), and then this line would just be removed.

---

But going forward, this means using the merge commit prevents different messages based on whether there were changes.

We can conditionally change the checkout so that on a closed event action, we checkout something different (like going back to PR head sha) but that brings us back to possibly having a wrong idea of whether there were changes (the stakes are lower though: at worst, a message might say that your docs changes have been incorporated when there were no changes).

We could checkout current target branch (main), with PR merged, to take the place of the "merge commit", and checkout "target branch - 1" to take place of the BASE commit... maybe that's even worse...

We could make this into an option, so that maintainers can choose the merge commit or the PR head sha, with the understanding that the latter may show changes that aren't there when rebases are needed, while the former cannot work on close...

this is just me trying to think through this, a brain dump, not necessarily well-thought-out ideas

[briantist]

I am not yet sure if this will be useful or not, but I've created a test workflow triggered on pull_request_target to dump context so we can see if there's anything useful.

Here's the test PR: briantist/gha-junk#2

Here are the workflow runs:

• opened: https://github.com/briantist/gha-junk/runs/4764364825?check_suite_focus=true
• closed (no merge): https://github.com/briantist/gha-junk/runs/4764416539?check_suite_focus=true
• reopened: https://github.com/briantist/gha-junk/runs/4764449339?check_suite_focus=true
• closed (merged): https://github.com/briantist/gha-junk/runs/4764459558?check_suite_focus=true

Nothing hugely groundbreaking stood out to me, but I did notice they al include the diff URL, like: https://patch-diff.githubusercontent.com/raw/briantist/gha-junk/pull/2.diff

If we use that and locally apply it to a checkout of the target branch (which is included in env/github context), I wonder if that gives us the same result as a merge commit? or is it no different from using the PR head sha? 🤔

I maybe cannot test further today.. but if you have any additional thoughts, happy to hear them.