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 b1617f00..d0d7521d 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 @@ -545,6 +545,14 @@ private String getMergeReadyComment(String commitMessage, List reviews) throw new UncheckedIOException(e); } + if (labels.stream().anyMatch(label -> workItem.bot.twoReviewersLabels().contains(label))) { + message.append("\n\n"); + message.append(":mag: One or more changes in this pull request modifies files in areas of "); + message.append("the source code that often require two reviewers. Please consider if this is "); + message.append("the case for this pull request, and if so, await a second reviewer to approve "); + message.append("this pull request before you integrate it."); + } + message.append("\n\n"); message.append("After integration, the commit message for the final commit will be:\n"); message.append("```\n"); 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 4767f9f4..6bc4343c 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 @@ -44,6 +44,7 @@ private final Map externalCommands; private final Map blockingCheckLabels; private final Set readyLabels; + private final Set twoReviewersLabels; private final Map readyComments; private final IssueProject issueProject; private final boolean ignoreStaleReviews; @@ -61,10 +62,10 @@ PullRequestBot(HostedRepository repo, HostedRepository censusRepo, String censusRef, LabelConfiguration labelConfiguration, Map externalCommands, Map blockingCheckLabels, Set readyLabels, - Map readyComments, IssueProject issueProject, boolean ignoreStaleReviews, - Set allowedIssueTypes, Pattern allowedTargetBranches, Path seedStorage, - HostedRepository confOverrideRepo, String confOverrideName, String confOverrideRef, - String censusLink) { + Set twoReviewersLabels, Map readyComments, IssueProject issueProject, + boolean ignoreStaleReviews, Set allowedIssueTypes, Pattern allowedTargetBranches, + Path seedStorage, HostedRepository confOverrideRepo, String confOverrideName, + String confOverrideRef, String censusLink) { remoteRepo = repo; this.censusRepo = censusRepo; this.censusRef = censusRef; @@ -72,6 +73,7 @@ this.externalCommands = externalCommands; this.blockingCheckLabels = blockingCheckLabels; this.readyLabels = readyLabels; + this.twoReviewersLabels = twoReviewersLabels; this.issueProject = issueProject; this.readyComments = readyComments; this.ignoreStaleReviews = ignoreStaleReviews; @@ -172,6 +174,10 @@ LabelConfiguration labelConfiguration() { return blockingCheckLabels; } + Set twoReviewersLabels() { + return twoReviewersLabels; + } + IssueProject issueProject() { return issueProject; } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotBuilder.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotBuilder.java index 562c8ead..40e2b116 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotBuilder.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotBuilder.java @@ -39,6 +39,7 @@ private Map externalCommands = Map.of(); private Map blockingCheckLabels = Map.of(); private Set readyLabels = Set.of(); + private Set twoReviewersLabels = Set.of(); private Map readyComments = Map.of(); private IssueProject issueProject = null; private boolean ignoreStaleReviews = false; @@ -88,6 +89,11 @@ public PullRequestBotBuilder readyLabels(Set readyLabels) { return this; } + public PullRequestBotBuilder twoReviewersLabels(Set twoReviewersLabels) { + this.twoReviewersLabels = twoReviewersLabels; + return this; + } + public PullRequestBotBuilder readyComments(Map readyComments) { this.readyComments = readyComments; return this; @@ -140,7 +146,7 @@ public PullRequestBotBuilder censusLink(String censusLink) { public PullRequestBot build() { return new PullRequestBot(repo, censusRepo, censusRef, labelConfiguration, externalCommands, - blockingCheckLabels, readyLabels, readyComments, issueProject, + blockingCheckLabels, readyLabels, twoReviewersLabels, readyComments, issueProject, ignoreStaleReviews, allowedIssueTypes, allowedTargetBranches, seedStorage, confOverrideRepo, confOverrideName, confOverrideRef, censusLink); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotFactory.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotFactory.java index 24f1bd24..3db22c83 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotFactory.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotFactory.java @@ -97,6 +97,13 @@ public String name() { } botBuilder.labelConfiguration(labelConfigurations.get(labelGroup)); } + if (repo.value().contains("two-reviewers")) { + var labels = repo.value().get("two-reviewers") + .stream() + .map(label -> label.asString()) + .collect(Collectors.toSet()); + botBuilder.twoReviewersLabels(labels); + } if (repo.value().contains("issues")) { botBuilder.issueProject(configuration.issueProject(repo.value().get("issues").asString())); } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java index c5bc6734..c54a57b7 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/LabelTests.java @@ -356,4 +356,58 @@ void stripSuffix(TestInfo testInfo) throws IOException { assertLastCommentContains(pr,"The `group` label was successfully added."); } } + + @Test + void twoReviewersLabels(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addReviewer(integrator.forge().currentUser().id()) + .addCommitter(author.forge().currentUser().id()); + var labelConfiguration = LabelConfigurationJson.builder() + .addMatchers("1", List.of(Pattern.compile("cpp$"))) + .addMatchers("2", List.of(Pattern.compile("hpp$"))) + .addGroup("group", List.of("1", "2")) + .addExtra("extra") + .build(); + var prBot = PullRequestBot.newBuilder() + .repo(integrator) + .censusRepo(censusBuilder.build()) + .twoReviewersLabels(Set.of("1")) + .labelConfiguration(labelConfiguration) + .build(); + + // Populate the projects repository + var localRepoFolder = tempFolder.path().resolve("localrepo"); + var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "123: This is a pull request"); + + // Add a label with -dev suffix + pr.addComment("/label add 1"); + TestBotRunner.runPeriodicItems(prBot); + + // The bot should reply with a success message + assertLastCommentContains(pr,"The `1` label was successfully added."); + + // Review the PR + var prAsReviewer = integrator.pullRequest(pr.id()); + prAsReviewer.addReview(Review.Verdict.APPROVED, "Looks good!"); + TestBotRunner.runPeriodicItems(prBot); + + // The bot should reply with a integration message + assertLastCommentContains(pr,"This change now passes all *automated* pre-integration checks"); + assertLastCommentContains(pr,":mag: One or more changes in this pull request modifies files"); + assertLastCommentContains(pr,"in areas of the source code that often require two reviewers."); + } + } }