search

neondatabase / autoscaling

Postgres vertical autoscaling in k8s
Apache License 2.0
166 stars 21 forks source link

CI: Add PR title checker #1087

Closed sharnoff closed 1 month ago

sharnoff commented 2 months ago

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.

mikhail-sakhnov commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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