PR build: use base ref instead of base sha to checkout BASE

[felixfontein]

I was wondering a long time why #14 didn't seem to help, since in particular in community.general every bump of galaxy.yml in main caused all PRs created before that version bump to have an error in the docs build (argument list too long on computing the diff), resp. in other collections to have a long list of changes. Example: https://github.com/ansible-collections/community.general/actions/runs/4052090683/jobs/6971113426

While staring at the problem again today I noticed that we're using base.sha, which is a fixed commit. It seems to be the commit the PR's branch was branched from (since it changes when the branch is rebased). But since HEAD is the merge commit on the latest version of the target branch, that commit is "too old". I'm not 100% sure whether base.ref is the correct value (I assume it happens to be main if the PR target branch is main, but I'm not 100% sure), but if it is then this should improve the situation.

[briantist]

I like the idea! I think we will have to do some extensive testing with external PRs pointed at your branch's version of the workflow, so we can see the behaviors; it's going to be a pain but probably necessary.

Also since we have a presentation coming up, I think I will prefer we wait until after that to merge, since current behavior is at least somewhat predictable 😅 but we can talk about that if you feel differently

[briantist]

The closed event should be using whatever the other events use, for consistency, the question is whether this is actually available in the event, we'll have to see

[felixfontein]

I would expect that github.event.pull_request.base.ref is always available. It could be though that (when multiple PRs have been merged closely to each other) that main already has more commits than the merge commit, so the diff computed on closing shows the removal of the newer commit(s). If there is no newer commit, the diff should be empty.

One thing I'm not sure about is whether the diff is actually needed after closing or merging the PR.

[felixfontein]

https://github.com/ansible-collections/community.docker/actions/runs/4085894401/jobs/7044478553 is a build after merging a PR. The bot comment was removed from ansible-collections/community.docker#574.

[briantist]

One thing I'm not sure about is whether the diff is actually needed after closing or merging the PR.

It's needed in order to provide a unique "closed"/"merged" event comment that only gets applied when there was a difference (as opposed to always posting/editing it). Without the diff, we'd either be adding a comment always, or we could opt out of changing the comment on close/merge.

Perhaps an alternative to calculating diffs on close/merge is to skip all builds, and only update a comment when one exists already.

IIRC, the logic and hoops we have to jump through in the workflows end up being more complicated but that might be worth it to 1) save CI time and 2) if it removes some events that don't have the ref we want.

[felixfontein]

I think avoiding the 'final' run of the docs build would be good, it saves a lot of CI resources (especially on large collections) and right now is the main problem for this PR, as I don't see a good way to do this correctly.

[briantist]

I think I agree.. I don't want to lose the functionality, so checking for an existing comment instead of building is a good compromise I think. The only question is whether that is the only thing preventing us from using a better ref, or if there's something else I forgot...

[briantist]

My comment here suggests that indeed only close is what prevents us from using the merge commit:
#38

Changing to use the merge branch in all cases except on PR close where it's not available.

[briantist]

actually now I am wondering whether that's even the same thing as this... I think I am confusing myself 😖

to quote you:

Bah, this is really a huge mess.

[felixfontein]

I changed community.docker to use my branch (ansible-collections/community.docker@0e1152a) and created a PR based on an older version of the c.d main branch (ansible-collections/community.docker@e87b327). At that point galaxy.yml said 3.4.0, while in current main it says 3.5.0.

[felixfontein]

The behavior for ansible-collections/community.docker#573 looks ok, but the behavior for #67 (comment) does not. Also there's the problem with main maybe being different than the version of the main branch used in the merge commit. But now I have to leave my train, so I'll stop for now :)