Add checks for commits in pull requests

quarkusio / quarkus-github-bot

A Quarkus-powered GitHub App to simplify issues and pull requests management in the Quarkus project.

Apache License 2.0

23 stars 24 forks source link

Add checks for commits in pull requests #236

Closed gsmet closed 1 year ago

gsmet commented 2 years ago

We should check that:

A PR does not contain merge commits (and prevent merging if so)
A PR does not contain a fixup commit (and prevent merging if so)

Using the check API is the way to go and @yrodiere implemented checks here: https://github.com/hibernate/hibernate-github-bot/tree/main/src/main/java/org/hibernate/infra/bot and it could serve as an inspiration.

nicolasgras commented 1 year ago

Hi @gsmet, I forked quarkus-github-bot repo and created a playground project in my account.

By using org.kohsuke:github-api library (https://github.com/hub4j/github-api), when a PR is opened or edited in the playground project, list of its commits is checked to know if there is any merge or fixup commit.

If yes, a check suite is created with a check run whose status is completed and conclusion is failure (see screenshot below).
If not, no check suite is created for this.

Let me know if it is ok for you, if yes I can open a PR for this issue.

gsmet commented 1 year ago

Hey @NicolasGras

Sure, you're very welcome to take over this issue. I think I would probably make a separate check for each issue instead of annotations, so that you can see very quickly what's going wrong.

I pointed to the work Yoann did and I think he also adds a comment to make sure the problems are noticed but that might not be necessary as a first step.

Feel free to open a PR so that we can discuss it with some code around.

Thanks!

nicolasgras commented 1 year ago

Hi @gsmet,

Thank you for your answer.

Do you mean:

1 check for each detected commit (merge or fixup commit) ? or
1 check for all merge commits and 1 check for all fixup commits ?

yrodiere commented 1 year ago

1 check for all merge commits and 1 check for all fixup commits ?

That one, I think :)

Also, I agree this sounds great, and I have a comment of my own:

If not, no check suite is created for this.

IMO it would be fine to create a check suite even when everything is alright.

Why: This would reassure maintainers that the bot did see the PR and did perform the checks. Which is not always a given as the bot might sometimes suffer some delays depending on the current load.

nicolasgras commented 1 year ago

Hi @yrodiere and @gsmet, I have opened a PR for this issue: https://github.com/quarkusio/quarkus-github-bot/pull/281. Please let me know if it is ok for you. Thanks :)