Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −1
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/_shared-docs-build-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,12 @@ jobs:
- name: Checkout BASE
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.base.sha }}
# Use base.ref, not base.sha, to make sure we do not get a checkout of the base at an earlier time
# (when the PR's branch was branched off the PR's target branch), but right now. Without this there
# will be a potentially large list of changes since the merge commit we check out for HEAD is based
# on a far newer version. I'm not 100% sure whether this is also a good idea for the closed event
# though...
Comment on lines +309 to +310
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ref: ${{ github.event.pull_request.base.ref }}
path: ${{ steps.vars.outputs.checkout-path }}
persist-credentials: false

Expand Down