search

Homebrew / actions

🚀 Homebrew's GitHub Actions
BSD 2-Clause "Simplified" License
123 stars 39 forks source link

check-commit-format: return failing status for non-compliant commits #346

Closed carlocab closed 1 year ago

carlocab commented 1 year ago

Let's make this check return a failing status whenever the PR has commits that don't follow our style guide. This will be a stronger nudge to contributors to fix up their PR branch.

This currently isn't a required check in Homebrew/core, so it shouldn't affect the rest of the CI run or our ability to merge PRs.

dawidd6 commented 1 year ago

Are we going to drop/deprecate the autosquash functionality of pr-pull?

carlocab commented 1 year ago

I don't think so; publish-commit-bottles still needs it.

MikeMcQuaid commented 1 year ago

Are we going to drop/deprecate the autosquash functionality of pr-pull?

IMO let's just lean into autosquash instead. I'm not sure I see why it's worth nudging users to change their commit messages if we can trivially fix it up ourselves.

carlocab commented 1 year ago

IMO let's just lean into autosquash instead. I'm not sure I see why it's worth nudging users to change their commit messages if we can trivially fix it up ourselves.

A few reasons:

  1. It's not really trivial, and still occasionally fails: https://github.com/Homebrew/homebrew-core/pull/128823#issuecomment-1515752655
  2. It destroys status check information. To see this, compare the status checks you see for this commit (not autosquashed), and this commit (autosquashed). The autosquashed commit has no associated status check information.
  3. The commit messages aren't great when it's not a version bump.

Destroying status check information wasn't such a big deal before since you could go back to the pull request and pull up the checks from there. But now that we push bottle commits to PR branches, those are no longer available either.

MikeMcQuaid commented 1 year ago

That's fair, thanks @carlocab. At a minimum it feels like autosquashing recording the existing commit hash in the commit message would be worthwhile.

carlocab commented 1 year ago

Superseded by #362.

For reference: I've fixed 1 and 2 in https://github.com/Homebrew/actions/pull/346#issuecomment-1515760663. 3 still happens, but those are relatively rare.