trino icon indicating copy to clipboard operation
trino copied to clipboard

Detect and block merge commits in CI

Open nineinchnick opened this issue 1 year ago • 3 comments

Description

Detect and block merge commits and commits with fixup! in the message.

Is this change a fix, improvement, new feature, refactoring, or other? improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific) ci

How would you describe this change to a non-technical end user or system administrator? n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed. ( ) Sufficient documentation is included in this PR. ( ) Documentation PR is available with #prnumber. ( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required. ( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

nineinchnick avatar Aug 04 '22 09:08 nineinchnick

Yes, we should block fixup commits too. If one of those goes in it’s probably a mistake — the author forgot to squash.

martint avatar Aug 04 '22 10:08 martint

Detect and block merge commits and commits with fixup! in the message.

This will make contributors think that fixup commits shouldn't be used, while we actually encourage people to use them, during the review. based on Starburst internal experience, this probaly won't have a positive net effect

let's focus on merge commits for now.

findepi avatar Aug 05 '22 21:08 findepi

Detect and block merge commits and commits with fixup! in the message.

This will make contributors think that fixup commits shouldn't be used, while we actually encourage people to use them, during the review. based on Starburst internal experience, this probaly won't have a positive net effect

let's focus on merge commits for now.

I added both behaviors, to fail on merge commits and to request changes on fixup commits. The Github Action adds reviews and it looks like this:

image

and after rebasing: image

Tested in https://github.com/nineinchnick/trino/pull/12

nineinchnick avatar Aug 08 '22 11:08 nineinchnick

See CI failure. Related.

hashhar avatar Aug 12 '22 17:08 hashhar

The two failures don't look related, I think it's ready to go now.

nineinchnick avatar Aug 15 '22 16:08 nineinchnick