diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java index 6ae227bf..b1617f00 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java @@ -170,9 +170,6 @@ private IssueProject issueProject() { ret.add(error); } - var headHash = pr.headHash(); - var originalCommits = localRepo.commitMetadata(baseHash, headHash); - for (var blocker : workItem.bot.blockingCheckLabels().entrySet()) { if (labels.contains(blocker.getKey())) { ret.add(blocker.getValue()); @@ -182,6 +179,41 @@ private IssueProject issueProject() { return ret; } + private Map blockingIntegrationLabels() { + return Map.of("rejected", "The change is currently blocked from integration by a rejection.", + "csr", "The change requires a CSR request to be approved."); + } + + private List botSpecificIntegrationBlockers() { + var ret = new ArrayList(); + + var issues = issues(); + var issueProject = issueProject(); + if (issueProject != null) { + for (var currentIssue : issues) { + try { + var iss = issueProject.issue(currentIssue.shortId()); + if (iss.isPresent()) { + if (!relaxedEquals(iss.get().title(), currentIssue.description())) { + var issueString = "[" + iss.get().id() + "](" + iss.get().webUrl() + ")"; + ret.add("Title mismatch between PR and JBS for issue " + issueString); + } + } else { + log.warning("Failed to retrieve information on issue " + currentIssue.id()); + } + } catch (RuntimeException e) { + log.warning("Temporary failure when trying to retrieve information on issue " + currentIssue.id()); + } + } + } + + labels.stream() + .filter(l -> blockingIntegrationLabels().containsKey(l)) + .forEach(l -> ret.add(blockingIntegrationLabels().get(l))); + + return ret; + } + private void updateCheckBuilder(CheckBuilder checkBuilder, PullRequestCheckIssueVisitor visitor, List additionalErrors) { if (visitor.isReadyForReview() && additionalErrors.isEmpty()) { checkBuilder.complete(true); @@ -266,7 +298,7 @@ private String getChecksList(PullRequestCheckIssueVisitor visitor) { .collect(Collectors.joining("\n")); } - private String getAdditionalErrorsList(List additionalErrors) { + private String warningListToText(List additionalErrors) { return additionalErrors.stream() .sorted() .map(err -> " ⚠️ " + err) @@ -337,7 +369,8 @@ private boolean relaxedEquals(String s1, String s2) { } - private String getStatusMessage(List comments, List reviews, PullRequestCheckIssueVisitor visitor, List additionalErrors) { + private String getStatusMessage(List comments, List reviews, PullRequestCheckIssueVisitor visitor, + List additionalErrors, List integrationBlockers) { var progressBody = new StringBuilder(); progressBody.append("---------\n"); progressBody.append("### Progress\n"); @@ -352,7 +385,16 @@ private String getStatusMessage(List comments, List reviews, Pu progressBody.append("s"); } progressBody.append("\n"); - progressBody.append(getAdditionalErrorsList(allAdditionalErrors)); + progressBody.append(warningListToText(allAdditionalErrors)); + } + + if (!integrationBlockers.isEmpty()) { + progressBody.append("\n\n### Integration blocker"); + if (integrationBlockers.size() > 1) { + progressBody.append("s"); + } + progressBody.append("\n"); + progressBody.append(warningListToText(integrationBlockers)); } var issues = issues(); @@ -724,8 +766,10 @@ private void checkStatus() { updateCheckBuilder(checkBuilder, visitor, additionalErrors); updateReadyForReview(visitor, additionalErrors); + var integrationBlockers = botSpecificIntegrationBlockers(); + // Calculate and update the status message if needed - var statusMessage = getStatusMessage(comments, activeReviews, visitor, additionalErrors); + var statusMessage = getStatusMessage(comments, activeReviews, visitor, additionalErrors, integrationBlockers); var updatedBody = updateStatusMessage(statusMessage); var title = pr.title(); @@ -739,7 +783,7 @@ private void checkStatus() { var commitMessage = String.join("\n", commit.message()); var readyForIntegration = visitor.messages().isEmpty() && additionalErrors.isEmpty() && - labels.stream().noneMatch(l -> workItem.bot.blockingReadyLabels().contains(l)); + integrationBlockers.isEmpty(); updateMergeReadyComment(readyForIntegration, commitMessage, comments, activeReviews, rebasePossible); if (readyForIntegration && rebasePossible) { diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java index 557405af..1f374591 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java @@ -78,11 +78,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } var labels = new HashSet<>(pr.labels()); - for (var blocker : bot.blockingIntegrationLabels().entrySet()) { - if (labels.contains(blocker.getKey())) { - reply.println(blocker.getValue()); - return; - } + if (!labels.contains("ready")) { + reply.println("This PR has not yet been marked as ready for integration."); + return; } // Run a final jcheck to ensure the change has been properly reviewed diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java index 7cb32d55..4767f9f4 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java @@ -172,22 +172,6 @@ LabelConfiguration labelConfiguration() { return blockingCheckLabels; } - Set blockingReadyLabels() { - return Set.of("csr"); - } - - Map blockingIntegrationLabels() { - return Map.of("rejected", "The change is currently blocked from integration by a rejection."); - } - - Set readyLabels() { - return readyLabels; - } - - Map readyComments() { - return readyComments; - } - IssueProject issueProject() { return issueProject; } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java index 293e05b0..5494204b 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java @@ -60,11 +60,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } var labels = new HashSet<>(pr.labels()); - for (var blocker : bot.blockingIntegrationLabels().entrySet()) { - if (labels.contains(blocker.getKey())) { - reply.println(blocker.getValue()); - return; - } + if (!labels.contains("ready")) { + reply.println("This PR has not yet been marked as ready for integration."); + return; } // Notify the author as well diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CSRTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CSRTests.java index 77720e07..d0c95512 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CSRTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CSRTests.java @@ -22,15 +22,11 @@ */ package org.openjdk.skara.bots.pr; +import org.junit.jupiter.api.*; import org.openjdk.skara.forge.Review; -import org.openjdk.skara.issuetracker.Comment; -import org.openjdk.skara.issuetracker.Link; -import org.openjdk.skara.issuetracker.Issue; -import org.openjdk.skara.test.*; -import org.openjdk.skara.vcs.Repository; +import org.openjdk.skara.issuetracker.*; import org.openjdk.skara.json.JSON; - -import org.junit.jupiter.api.*; +import org.openjdk.skara.test.*; import java.io.IOException; import java.util.*; @@ -527,6 +523,9 @@ void csrLabelShouldBlockReadyLabel(TestInfo testInfo) throws IOException { // PR should not be ready prAsAuthor = author.pullRequest(pr.id()); assertFalse(prAsAuthor.labels().contains("ready")); + + // The body should contain a note about why + assertTrue(pr.body().contains("change requires a CSR request to be approved")); } } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java index da1a699b..2f59b5e0 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java @@ -935,6 +935,7 @@ void issueInSummary(TestInfo testInfo) throws IOException { // The PR title does not match the issue title assertTrue(pr.body().contains("Title mismatch")); + assertTrue(pr.body().contains("Integration blocker")); // Correct it pr.setTitle(issue2.id() + " - " + issue2.title()); @@ -942,6 +943,7 @@ void issueInSummary(TestInfo testInfo) throws IOException { // Check the status again - it should now match TestBotRunner.runPeriodicItems(checkBot); assertFalse(pr.body().contains("Title mismatch")); + assertFalse(pr.body().contains("Integration blocker")); // Use an invalid issue key var issueKey = issue1.id().replace("TEST", "BADPROJECT"); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java index d06bc43f..c9f9948a 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java @@ -22,18 +22,15 @@ */ package org.openjdk.skara.bots.pr; +import org.junit.jupiter.api.*; import org.openjdk.skara.forge.*; -import org.openjdk.skara.issuetracker.Comment; import org.openjdk.skara.test.*; import org.openjdk.skara.vcs.Repository; -import org.junit.jupiter.api.*; - import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.*; import java.util.Set; -import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.*; import static org.openjdk.skara.bots.pr.PullRequestAsserts.assertLastCommentContains; @@ -235,11 +232,7 @@ void notReviewed(TestInfo testInfo) throws IOException { TestBotRunner.runPeriodicItems(mergeBot); // The bot should reply with an error message - var error = pr.comments().stream() - .filter(comment -> comment.body().contains("integration request cannot be fulfilled at this time")) - .filter(comment -> comment.body().contains("failed the final jcheck")) - .count(); - assertEquals(1, error, pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n---\n"))); + assertLastCommentContains(pr, "PR has not yet been marked as ready for integration"); } } @@ -639,16 +632,20 @@ void cannotRebase(TestInfo testInfo) throws IOException { var conflictingHash = CheckableRepository.appendAndCommit(localRepo, "This looks like a conflict"); localRepo.push(conflictingHash, author.url(), "master"); + // Trigger a new check run + pr.setBody(pr.body() + " recheck"); + TestBotRunner.runPeriodicItems(mergeBot); + + // The bot should reply with an error message + assertLastCommentContains(pr, "this pull request can not be integrated"); + // Attempt an integration pr.addComment("/integrate"); TestBotRunner.runPeriodicItems(mergeBot); // The bot should reply with an error message - var error = pr.comments().stream() - .filter(comment -> comment.body().contains("It was not possible to rebase your changes automatically.")) - .filter(comment -> comment.body().contains("Please merge `master`")) - .count(); - assertEquals(1, error); + TestBotRunner.runPeriodicItems(mergeBot); + assertLastCommentContains(pr, "PR has not yet been marked as ready for integration"); } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java index 7875c4ac..fbfcbe4e 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java @@ -179,7 +179,7 @@ void noIntegration(TestInfo testInfo) throws IOException { // It should not be possible to integrate yet pr.addComment("/integrate"); TestBotRunner.runPeriodicItems(prBot); - assertLastCommentContains(reviewerPr,"Too few reviewers with at least role author found (have 0, need at least 1)"); + assertLastCommentContains(reviewerPr,"PR has not yet been marked as ready for integration"); // Relax the requirement reviewerPr.addComment("/reviewers 1"); @@ -239,7 +239,7 @@ void noSponsoring(TestInfo testInfo) throws IOException { // It should not be possible to sponsor reviewerPr.addComment("/sponsor"); TestBotRunner.runPeriodicItems(prBot); - assertLastCommentContains(reviewerPr,"Too few reviewers with at least role author found (have 0, need at least 1)"); + assertLastCommentContains(reviewerPr,"PR has not yet been marked as ready for integration"); // Relax the requirement reviewerPr.addComment("/reviewers 1"); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java index 799e105a..6cbec6a7 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java @@ -523,11 +523,7 @@ void sponsorAfterFailingCheck(TestInfo testInfo) throws IOException { TestBotRunner.runPeriodicItems(mergeBot); // The bot should reply with an error message - var error = pr.comments().stream() - .filter(comment -> comment.body().contains("integration request cannot be fulfilled at this time")) - .filter(comment -> comment.body().contains("failed the final jcheck")) - .count(); - assertEquals(1, error); + assertLastCommentContains(pr, "PR has not yet been marked as ready for integration"); // Make it ready for integration again approvalPr.addReview(Review.Verdict.APPROVED, "Sorry, wrong button"); @@ -600,17 +596,20 @@ void cannotRebase(TestInfo testInfo) throws IOException { var conflictingHash = CheckableRepository.appendAndCommit(localRepo, "This looks like a conflict"); localRepo.push(conflictingHash, author.url(), "master"); + // Trigger a new check run + pr.setBody(pr.body() + " recheck"); + TestBotRunner.runPeriodicItems(mergeBot); + + // The bot should reply with an error message + assertLastCommentContains(pr, "this pull request can not be integrated"); + // Reviewer now agrees to sponsor var reviewerPr = reviewer.pullRequest(pr.id()); reviewerPr.addComment("/sponsor"); - TestBotRunner.runPeriodicItems(mergeBot); // The bot should reply with an error message - var error = pr.comments().stream() - .filter(comment -> comment.body().contains("It was not possible to rebase your changes automatically.")) - .filter(comment -> comment.body().contains("Please merge `master`")) - .count(); - assertEquals(1, error); + TestBotRunner.runPeriodicItems(mergeBot); + assertLastCommentContains(pr, "PR has not yet been marked as ready for integration"); } }