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

Update all uses of actions/checkout to use explicit ref #3322

Merged
merged 2 commits into from Sep 14, 2023

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Sep 14, 2023

What

actions/checkout uses GITHUB_SHA by default for the ref which it checks out. This sometimes leads to confusing issues, because the pull_request commit actually operates not on the latest commit of a pull request, but on the latest merge commit of a pull request. This causes a lot of confusion and even actual bugs.

In this PR, I'm setting all the ref values explicitly to whatever is available:

  • Some reusable workflows already require a commit, .e.g reusable_build_and_publish_web has a release-commit input
  • If the workflow can be triggered from a PR, then we use github.event.pull_request.head.ref, which points to the HEAD of the PR branch.
  • In all other cases we fall back to github.sha. For example a push to main will still use github.sha, which is fine.

There are caveats. With these changes, we no longer test PRs on the merge commit. That means if your PR is out-of-date with the target branch (e.g. main), and someone pushes to main, you are less likely to find out about any potential incompatibility if it doesn't cause a merge conflict. This means we need to be careful about merging without being up-to-date with main. Worst case scenario it will result in more failures on main which we'll be notified about on Slack.

The main benefit of this is less confusion about which commit we're on when viewing the web demo, which is an essential part of our overall workflow at this point. Once we get over the source_hash situation and have the ability to turn on merge queue, we'll once again have much more "main safety".

Before:
image

After:
image

Checklist

@jprochazk jprochazk added the 🧑‍💻 dev experience developer experience (excluding CI) label Sep 14, 2023
@Wumpf Wumpf self-requested a review September 14, 2023 17:24
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

let's do this!

@Wumpf Wumpf merged commit 097b419 into main Sep 14, 2023
27 of 28 checks passed
@Wumpf Wumpf deleted the jan/ignore-merge-commits branch September 14, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants