A PR diff may show docs changes if the source branch is not based on the latest target commit

[briantist]

For example, if a PR is created from a branch that based on main, and a new commit to main after branching changes docs, then the PR docs build will show that the PR changes the docs, even though that is not the case. In fact, the docs in main are ahead of what's in the PR, and ideally the PR's branch would be rebased, however it is not always necessary to rebase a branch when there are otherwise no conflicts.

The reason this happens is that in the PR workflow, we build the docs for both the BASE and HEAD commit of the PR. If the PR is against main for example, we build docs for main, but since the PR is based on an earlier main commit, the output will be different.

One thing we can try to do, is do our BASE build against the commit that the PR branch was started from. That would solve the situation laid out above.

But in another case, consider that the PR actually did change docs, but main is now ahead of the branch with its on changes. If we build against the PR branch's original base, we will showing the (intended) changes of the PR, but will not necessarily be representing the sate of the docs after the PR is merged.

Although GitHub will alert to conflicts in the tracked files, the rendered docsite is not tracked.

I'm not sure what the best or correct course of action is.

I don't think it's a good idea to do 3 builds, comparing the PR to both its own base and to its merge target, it seems a little too complicated.

A third option, whether the base render is based on latest target or on branch's base, is to detect that the branch has drifted, and include a warning in the PR comment that the differences shown may not be accurate, recommending a rebase if accuracy is a concern.

[felixfontein]

I've tried reading up on this, because I thought the checkout action already does some rebasing/merging. According to the documentation of the pull_request event (https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#pull_request) the event seems to contain information on the merge commit (and the commit won't happen if there is a merge conflict); this is also confirmed by the explanation for the pull_request_target event (https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#pull_request_target). So I think this should already work. But then I don't understand why we observed something else in the community.docker repo :)

[felixfontein]

For reference, here is one PRs where this happened:

• Fix unintended breaking change by vendoring distutils.version ansible-collections/community.docker#269 (comment)

[felixfontein]

The problem might be that we're explicitly requesting the checkout of the HEAD's commit by specifying github.event.pull_request.head.sha. Simpliy leaving the ref away might solve that problem. I've created a branch for it: main...felixfontein:fix-merge right now I'm not sure how to test it, I probably first have to create a docs PR in community.docker so then I can create another PR (which uses my branch of the workflow) to see what happens...

[felixfontein]

Hmm, testing seems to be more tricky. I've created a PR which is based of an older version of main (after merging a docs PR) that uses my branch, but it still uses the workflow mentioned in main and not the one mentioned in the PR. So to test a workflow update, it looks like first the workflow has to be changed to the modified one in main before it can be tested. That's... not awesome.

[felixfontein]

#14 fixes this. It's not a perfect fix (see the comment it adds), but I'm not sure how to improve it...