Skip to content

Detect and block merge commits in CI#13494

Merged
hashhar merged 1 commit intotrinodb:masterfrom
nineinchnick:block-merge-commits
Aug 16, 2022
Merged

Detect and block merge commits in CI#13494
hashhar merged 1 commit intotrinodb:masterfrom
nineinchnick:block-merge-commits

Conversation

@nineinchnick
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick commented Aug 4, 2022

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`)

@cla-bot cla-bot bot added the cla-signed label Aug 4, 2022
@nineinchnick nineinchnick mentioned this pull request Aug 4, 2022
@martint
Copy link
Copy Markdown
Member

martint commented Aug 4, 2022

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

@nineinchnick nineinchnick requested review from findepi and martint August 4, 2022 18:13
- name: Test Docker Image
run: core/docker/build.sh
- name: Block Merge Commits
uses: nineinchnick/block-commits-action@cea2eb9b1b83c500d877cd60649d98b58fc2423c
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we put this somewhere under our github org?
Do we need a new repo, for actions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In #12817 I have a few actions that I added to nineinchnick/github-actions and I suggested to transfer this repo to the trinodb org. If you'd create it, I could add this action there too.

@findepi
Copy link
Copy Markdown
Member

findepi commented Aug 5, 2022

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.

@nineinchnick nineinchnick force-pushed the block-merge-commits branch 5 times, most recently from 73b0a1d to 0504aac Compare August 8, 2022 11:10
@nineinchnick
Copy link
Copy Markdown
Member Author

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 nineinchnick#12

@nineinchnick nineinchnick requested review from findepi and hashhar August 8, 2022 11:20
steps:
- uses: actions/checkout@v3
- name: Check Commits
uses: nineinchnick/block-commits-action@cf4e63bde529c78a14e987b6648046e0b815305a
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Aug 12, 2022

See CI failure. Related.

Merge commits will cause the pipeline to fail, because they're never
allowed. Fixup commits on the other hand are helpful during longer
reviews and shouldn't cause the run to fail. Instead, the action creates
a review and requests changes.
@nineinchnick
Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Looks good. We can iterate from here.

Thanks @nineinchnick.

@hashhar hashhar merged commit 967b045 into trinodb:master Aug 16, 2022
@github-actions github-actions bot added this to the 393 milestone Aug 16, 2022
@nineinchnick nineinchnick deleted the block-merge-commits branch November 2, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants