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

CI: Add PR title checker #1087

Merged
merged 4 commits into from
Oct 1, 2024
Merged

CI: Add PR title checker #1087

merged 4 commits into from
Oct 1, 2024

Conversation

sharnoff
Copy link
Member

@sharnoff sharnoff commented Sep 25, 2024

Loosely adapted the workflow from what we have in neondatabase/cloud, but with the format changed to match what we've previously done in this repo.

As currently implemented, PR titles would generally be required to look like:

component: Short message

or, at its most flexible:

component1/subsystem,component2/subsystem: Short message

There's two escape hatches:

  1. If the PR title starts with Revert: "
  2. If you include <!-- affects all --> in the PR body — in case your PR is more broad

Some context:

  1. When looking at recent commits, I find it very useful to be able to quickly determine the scope of previous changes.
  2. Historically, the existing PR title format was maintained as a "best effort" sort of situation, with some exceptions slipping through
  3. Recently, there's been a few occasions where I've asked for a PR title to be changed to match the existing convention.

It's better to have something automated require this, so this PR adds that.


Notes for review: This PR is mostly for discussion! We can change the existing convention if we want :)

I do think that enforcing a common format is valuable regardless, even if it's not the particular implementation initially used in this PR.

Loosely adapted the workflow from what we have in neondatabase/cloud,
but with the format changed to match what we've previously done in this
repo.
@sharnoff sharnoff changed the title Add PR title checker CI: Add PR title checker Sep 25, 2024
Meant to include this; forgot!
@mikhail-sakhnov
Copy link
Contributor

Broadly looks good and I really like the idea. What do you think if we extract shell script from yaml to a separate files under scripts directory? We could re use them locally, for example by using https://pre-commit.com/

@sharnoff
Copy link
Member Author

Hm, looks like we'd need a commit-msg hook instead of a pre-commit hook. I'm not so sure about it tbh -- especially because it's ok to have very simple commits in the branch, as long as they get squashed :)

Would be good to discuss?

@mikhail-sakhnov
Copy link
Contributor

I mostly used reference to pre commit as an example why to extract script to external file.

though if we look on your point: pre commit hooks are skippable and the tool I mentioned is optional. E.g even you have config for it in the repo, you still need explicitly turn it on.

@sharnoff sharnoff marked this pull request as ready for review October 1, 2024 14:04
@sharnoff
Copy link
Member Author

sharnoff commented Oct 1, 2024

Planning to merge as-is, but we should discuss a pre-commit hook or something after :)

@sharnoff sharnoff merged commit ecdb9ed into main Oct 1, 2024
22 checks passed
@sharnoff sharnoff deleted the sharnoff/pr-title-check branch October 1, 2024 17:44
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