search

ansible-community / github-docs-build

GitHub actions and workflows for building Ansible collection documentation.
GNU General Public License v3.0
10 stars 6 forks source link

Use merge commit for PR instead of HEAD commit of PR #14

Closed felixfontein closed 2 years ago

felixfontein commented 2 years ago

Fixes #3. See https://github.com/ansible-collections/community.docker/pull/280 as an example; it was run with https://github.com/ansible-collections/community.docker/commit/c4a009a5375793a750584752868751e535ee510c.

felixfontein commented 2 years ago

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

felixfontein commented 2 years ago

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 commented 2 years ago

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

briantist commented 2 years ago

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:

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 commented 2 years ago

@briantist thanks!

felixfontein commented 2 years ago

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 commented 2 years ago

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

briantist commented 2 years ago

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 commented 2 years ago

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: https://github.com/briantist/gha-junk/pull/2

Here are the workflow runs:

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.