Skip to content
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.

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

Upgrade actions/checkout to v3 #1715

Merged
merged 3 commits into from
Mar 7, 2022
Merged

Upgrade actions/checkout to v3 #1715

merged 3 commits into from
Mar 7, 2022

Conversation

cbush
Copy link
Collaborator

@cbush cbush commented Mar 7, 2022

Checking if v3 supports the pull_request_target event to actually check out the PR branch by default

@mongodben
Copy link
Collaborator

mongodben commented Mar 7, 2022

based on the docs for v3, seems like it'd get the HEAD commit for the branch that triggers the workflow. relevant documentation:

Screen Shot 2022-03-07 at 11 27 52 AM

i also see this subsection of docs saying that that you can explicitly specify the 'pull request HEAD commit instead of merge commit' - https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit
not sure what this means but seems as if it might be relevant.

also, are there any expected changes to behavior of the GH actions?

@cbush
Copy link
Collaborator Author

cbush commented Mar 7, 2022

Exactly why I didn't add the second snippet ("PR head").

Everything was working as expected until we started using the (relatively new) pull_request_target event.

Didn't notice @nathan-contino-mongo's issue because our tests didn't actually modify the docusaurus source in any meaningful way.

So, my hypothesis is that <v3 just doesn't handle pull_request_target correctly. By upgrading, I'm hoping it will go back to the way it says it works in the docs, which is that by default (i.e. without the ref option) the checked out branch is the PR ref.

I don't expect any impact on the other workflows.

Copy link
Collaborator

@mongodben mongodben left a comment

Choose a reason for hiding this comment

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

lgtm.

we should prob test out the pull_request_target action further as well.

Copy link
Contributor

@nathan-contino-mongo nathan-contino-mongo left a comment

Choose a reason for hiding this comment

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

LGTM

@cbush cbush merged commit e724cbb into master Mar 7, 2022
@cbush cbush deleted the actions-v3 branch March 7, 2022 16:50
@cbush
Copy link
Collaborator Author

cbush commented Mar 7, 2022

Boo, turns out my hypothesis was incorrect! Going with solution here: actions/checkout#518 (comment)

Since we want the same "default" behavior of checkout on the pull_request event, we're checking out the merge commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants