From efafe4c2af680c3475d52b8e0632ee6017e0fd87 Mon Sep 17 00:00:00 2001 From: Julien Ponge Date: Thu, 26 Mar 2026 16:20:45 +0100 Subject: [PATCH] Post comment advising squash merge if merge commits are found This still reports a failing check when a PR branch contains merge commits, but it reminds maintainers about the possibility to use a squashed commit merge if the repository and branch protection rules allow it. --- .../CheckPullRequestContributionRules.java | 33 ++++++ .../java/io/quarkus/bot/util/Strings.java | 1 + ...CheckPullRequestContributionRulesTest.java | 104 ++++++++++++++++++ 3 files changed, 138 insertions(+) diff --git a/src/main/java/io/quarkus/bot/CheckPullRequestContributionRules.java b/src/main/java/io/quarkus/bot/CheckPullRequestContributionRules.java index 0e9220dc..09f94c97 100644 --- a/src/main/java/io/quarkus/bot/CheckPullRequestContributionRules.java +++ b/src/main/java/io/quarkus/bot/CheckPullRequestContributionRules.java @@ -5,6 +5,7 @@ import java.util.ArrayList; import java.util.Date; import java.util.List; +import java.util.Optional; import jakarta.inject.Inject; @@ -17,6 +18,7 @@ import org.kohsuke.github.GHCheckRunBuilder.Output; import org.kohsuke.github.GHCommit; import org.kohsuke.github.GHEventPayload; +import org.kohsuke.github.GHIssueComment; import org.kohsuke.github.GHPullRequest; import org.kohsuke.github.GHPullRequestCommitDetail; import org.kohsuke.github.GHRepository; @@ -26,6 +28,8 @@ import io.quarkus.bot.config.Feature; import io.quarkus.bot.config.QuarkusGitHubBotConfig; import io.quarkus.bot.config.QuarkusGitHubBotConfigFile; +import io.quarkus.bot.service.GHIssueCommentService; +import io.quarkus.bot.util.Strings; public class CheckPullRequestContributionRules { @@ -36,6 +40,12 @@ public class CheckPullRequestContributionRules { public static final String MERGE_COMMIT_CHECK_RUN_NAME = "Check Pull Request - Merge commits"; public static final String MERGE_COMMIT_ERROR_OUTPUT_TITLE = "PR contains merge commits"; public static final String MERGE_COMMIT_ERROR_OUTPUT_SUMMARY = "Pull request that contains merge commits can not be merged"; + public static final String MERGE_COMMIT_COMMENT_MESSAGE = """ + \u26a0\ufe0f **This pull request contains merge commits.** + + This is acceptable provided that it is **squash merged**. + + Maintainers: please use the **Squash and merge** option when merging this pull request."""; public static final String FIXUP_COMMIT_CHECK_RUN_NAME = "Check Pull Request - Fixup commits"; public static final String FIXUP_COMMIT_ERROR_OUTPUT_TITLE = "PR contains fixup commits"; @@ -47,6 +57,9 @@ public class CheckPullRequestContributionRules { @Inject QuarkusGitHubBotConfig quarkusBotConfig; + @Inject + GHIssueCommentService commentService; + void checkPullRequestContributionRules( @PullRequest.Opened @PullRequest.Reopened @PullRequest.Synchronize GHEventPayload.PullRequest pullRequestPayload, @ConfigFile("quarkus-github-bot.yml") QuarkusGitHubBotConfigFile quarkusBotConfigFile) throws IOException { @@ -67,6 +80,9 @@ void checkPullRequestContributionRules( buildCheckRun(checkCommitData.getMergeCommitDetails(), repostitory, headCommit, MERGE_COMMIT_CHECK_RUN_NAME, MERGE_COMMIT_ERROR_OUTPUT_TITLE, MERGE_COMMIT_ERROR_OUTPUT_SUMMARY); + // Post comment advising squash merge if merge commits are found + handleMergeCommitComment(pullRequest, checkCommitData.getMergeCommitDetails()); + // Fixup commits buildCheckRun(checkCommitData.getFixupCommitDetails(), repostitory, headCommit, FIXUP_COMMIT_CHECK_RUN_NAME, FIXUP_COMMIT_ERROR_OUTPUT_TITLE, FIXUP_COMMIT_ERROR_OUTPUT_SUMMARY); @@ -164,6 +180,23 @@ public static void buildCheckRun(List commitDetails, } } + private void handleMergeCommitComment(GHPullRequest pullRequest, + List mergeCommitDetails) throws IOException { + Optional existingComment = commentService.findBotCommentInIssue( + pullRequest, Strings.MERGE_COMMIT_COMMENT_MARKER); + + if (!mergeCommitDetails.isEmpty()) { + if (existingComment.isEmpty()) { + String formattedComment = Strings.commentByBot(MERGE_COMMIT_COMMENT_MESSAGE) + + Strings.MERGE_COMMIT_COMMENT_MARKER; + pullRequest.comment(formattedComment); + } + } else { + existingComment.ifPresent(comment -> commentService.deleteComment( + comment, pullRequest.getNumber(), false)); + } + } + private static String buildDryRunLogMessage(List commitDetails, String checkRunName, String errorOutputSummary) { StringBuilder comment = new StringBuilder(); diff --git a/src/main/java/io/quarkus/bot/util/Strings.java b/src/main/java/io/quarkus/bot/util/Strings.java index 7f3e2a16..75b3cceb 100644 --- a/src/main/java/io/quarkus/bot/util/Strings.java +++ b/src/main/java/io/quarkus/bot/util/Strings.java @@ -2,6 +2,7 @@ public class Strings { public static final String EDITORIAL_RULES_COMMENT_MARKER = "\n"; + public static final String MERGE_COMMIT_COMMENT_MARKER = "\n"; public static boolean isNotBlank(String string) { return string != null && !string.trim().isEmpty(); diff --git a/src/test/java/io/quarkus/bot/it/CheckPullRequestContributionRulesTest.java b/src/test/java/io/quarkus/bot/it/CheckPullRequestContributionRulesTest.java index 55668232..4af635c5 100644 --- a/src/test/java/io/quarkus/bot/it/CheckPullRequestContributionRulesTest.java +++ b/src/test/java/io/quarkus/bot/it/CheckPullRequestContributionRulesTest.java @@ -2,6 +2,7 @@ import static io.quarkiverse.githubapp.testing.GitHubAppTesting.given; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.contains; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -10,6 +11,7 @@ import static org.mockito.Mockito.when; import java.io.IOException; +import java.util.List; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -19,6 +21,7 @@ import org.kohsuke.github.GHCommit; import org.kohsuke.github.GHCommitPointer; import org.kohsuke.github.GHEvent; +import org.kohsuke.github.GHIssueComment; import org.kohsuke.github.GHPullRequest; import org.kohsuke.github.GHPullRequestCommitDetail; import org.kohsuke.github.GHRepository; @@ -80,6 +83,7 @@ void pullRequestHasTwoCheckRuns() throws IOException { mockPR = mocks.pullRequest(samplePullRequestId); when(mockPR.getRepository()).thenReturn(mockRepo); + when(mockPR.getComments()).thenReturn(List.of()); setupMockHeadCommit(); @@ -101,6 +105,8 @@ void pullRequestHasTwoCheckRuns() throws IOException { verify(mockCheckRunBuilder, times(2)).withConclusion(eq(GHCheckRun.Conclusion.SUCCESS)); verify(mockCheckRunBuilder, times(2)).create(); + verify(mockPR, times(1)).getComments(); + verifyNoMoreInteractions(mocks.ghObjects()); }); } @@ -119,6 +125,7 @@ void pullRequestHasOneCheckRunSucessAndOneCheckRunFailIfMergeCommit() throws IOE mockPR = mocks.pullRequest(samplePullRequestId); when(mockPR.getRepository()).thenReturn(mockRepo); + when(mockPR.getComments()).thenReturn(List.of()); setupMockHeadCommit(); @@ -143,10 +150,41 @@ void pullRequestHasOneCheckRunSucessAndOneCheckRunFailIfMergeCommit() throws IOE verify(mockCheckRunBuilder, times(1)).add(any(Output.class)); verify(mockCheckRunBuilder, times(2)).create(); + verify(mockPR, times(1)).getComments(); + verify(mockPR, times(1)).comment(contains("squash merged")); + verifyNoMoreInteractions(mocks.ghObjects()); }); } + /** + * If PR contains merge commits and no existing bot comment, + * a comment advising squash merge is posted. + */ + @Test + void pullRequestWithMergeCommitPostsSquashMergeComment() throws IOException { + + setupMock(); + + given().github(mocks -> { + mocks.configFile("quarkus-github-bot.yml").fromString("features: [ CHECK_CONTRIBUTION_RULES ]\n"); + + mockPR = mocks.pullRequest(samplePullRequestId); + when(mockPR.getRepository()).thenReturn(mockRepo); + when(mockPR.getComments()).thenReturn(List.of()); + + setupMockHeadCommit(); + + GHPullRequestCommitDetail mockMergeCommitDetail = setupMockMergeCommit(); + PagedIterable iterableMock = MockHelper.mockPagedIterable(mockMergeCommitDetail); + when(mockPR.listCommits()).thenReturn(iterableMock); + }) + .when().payloadFromString(getSamplePullRequestPayload()) + .event(GHEvent.PULL_REQUEST) + .then().github(mocks -> verify(mockPR, times(1)) + .comment(contains("squash merged"))); + } + /** * If PR contain 1 fixup commit and not any merge commit, * then 2 checks run are created in PR: 1 in FAILURE and 1 in SUCESS @@ -161,6 +199,7 @@ void pullRequestHasOneCheckRunSucessAndOneCheckRunFailIfFixupCommit() throws IOE mockPR = mocks.pullRequest(samplePullRequestId); when(mockPR.getRepository()).thenReturn(mockRepo); + when(mockPR.getComments()).thenReturn(List.of()); setupMockHeadCommit(); @@ -185,10 +224,75 @@ void pullRequestHasOneCheckRunSucessAndOneCheckRunFailIfFixupCommit() throws IOE verify(mockCheckRunBuilder, times(1)).add(any(Output.class)); verify(mockCheckRunBuilder, times(2)).create(); + verify(mockPR, times(1)).getComments(); + verifyNoMoreInteractions(mocks.ghObjects()); }); } + /** + * If PR no longer contains merge commits but a bot comment exists, + * the comment is deleted. + */ + @Test + void pullRequestWithoutMergeCommitDeletesExistingComment() throws IOException { + + setupMock(); + + GHIssueComment mockComment = mock(GHIssueComment.class); + when(mockComment.getBody()).thenReturn( + "old comment" + io.quarkus.bot.util.Strings.MERGE_COMMIT_COMMENT_MARKER); + + given().github(mocks -> { + mocks.configFile("quarkus-github-bot.yml").fromString("features: [ CHECK_CONTRIBUTION_RULES ]\n"); + + mockPR = mocks.pullRequest(samplePullRequestId); + when(mockPR.getRepository()).thenReturn(mockRepo); + when(mockPR.getComments()).thenReturn(List.of(mockComment)); + + setupMockHeadCommit(); + + PagedIterable iterableMock = MockHelper.mockPagedIterable(); + when(mockPR.listCommits()).thenReturn(iterableMock); + }) + .when().payloadFromString(getSamplePullRequestPayload()) + .event(GHEvent.PULL_REQUEST) + .then().github(mocks -> verify(mockComment, times(1)) + .delete()); + } + + /** + * If PR contains merge commits and a bot comment already exists, + * no new comment is posted (idempotent). + */ + @Test + void pullRequestWithMergeCommitDoesNotDuplicateComment() throws IOException { + + setupMock(); + + GHIssueComment mockComment = mock(GHIssueComment.class); + when(mockComment.getBody()).thenReturn( + "existing comment" + io.quarkus.bot.util.Strings.MERGE_COMMIT_COMMENT_MARKER); + + given().github(mocks -> { + mocks.configFile("quarkus-github-bot.yml").fromString("features: [ CHECK_CONTRIBUTION_RULES ]\n"); + + mockPR = mocks.pullRequest(samplePullRequestId); + when(mockPR.getRepository()).thenReturn(mockRepo); + when(mockPR.getComments()).thenReturn(List.of(mockComment)); + + setupMockHeadCommit(); + + GHPullRequestCommitDetail mockMergeCommitDetail = setupMockMergeCommit(); + PagedIterable iterableMock = MockHelper.mockPagedIterable(mockMergeCommitDetail); + when(mockPR.listCommits()).thenReturn(iterableMock); + }) + .when().payloadFromString(getSamplePullRequestPayload()) + .event(GHEvent.PULL_REQUEST) + .then().github(mocks -> verify(mockPR, times(0)) + .comment(any(String.class))); + } + private static long samplePullRequestId = 1091703530; private static String sampleHeadCommitSha = "7277231f08d6641edbdc07ee327dca1cc11e754d";