Updated after review

bots/pr/src/main/java/org/openjdk/skara/bots/pr/AdditionalConfiguration.java

@@ -24,16 +24,40 @@
 
 import org.openjdk.skara.host.HostUser;
 import org.openjdk.skara.issuetracker.Comment;
+import org.openjdk.skara.jcheck.JCheckConfiguration;
+import org.openjdk.skara.vcs.*;
 
+import java.io.IOException;
 import java.util.*;
 
 public class AdditionalConfiguration {
-    static List<String> get(HostUser botUser, List<Comment> comments) {
+    static List<String> get(ReadOnlyRepository repository, Hash hash, HostUser botUser, List<Comment> comments) throws IOException {
+        var currentConfiguration = JCheckConfiguration.from(repository, hash);
+        var currentReviewers = currentConfiguration.checks().reviewers();
         var ret = new ArrayList<String>();
-        var requiredReviewersOverride = ReviewersTracker.currentRequiredReviewers(botUser, comments);
-        if (requiredReviewersOverride.isPresent()) {
+        var additionalReviewers = ReviewersTracker.additionalRequiredReviewers(botUser, comments);
+        for (var additionalReviewer : additionalReviewers.entrySet()) {
             ret.add("[checks \"reviewers\"]");
-            ret.add("minimum=" + requiredReviewersOverride.get());
+            var role = additionalReviewer.getKey();
+            switch (role) {
+                case "lead":
+                    ret.add("lead=" + (currentReviewers.lead() + additionalReviewer.getValue()));
+                    break;
+                case "reviewers":
+                    ret.add("reviewers=" + (currentReviewers.reviewers() + additionalReviewer.getValue()));
+                    break;
+                case "committers":
+                    ret.add("committers=" + (currentReviewers.committers() + additionalReviewer.getValue()));
+                    break;
+                case "authors":
+                    ret.add("authors=" + (currentReviewers.authors() + additionalReviewer.getValue()));
+                    break;
+                case "contributors":
+                    ret.add("contributors=" + (currentReviewers.contributors() + additionalReviewer.getValue()));
+                    break;
+                default:
+                    break;
+            }
         }
         return ret;
     }

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java

@@ -482,7 +482,8 @@ private void checkStatus() {
                 }
 
                 // Determine current status
-                prInstance.executeChecks(localHash, censusInstance, visitor, AdditionalConfiguration.get(pr.repository().forge().currentUser(), comments));
+                var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), comments);
+                prInstance.executeChecks(localHash, censusInstance, visitor, additionalConfiguration);
                 additionalErrors = botSpecificChecks();
             }
             else {

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java

@@ -44,7 +44,7 @@ class CheckWorkItem extends PullRequestWorkItem {
     private final Map<String, String> blockingLabels;
     private final IssueProject issueProject;
 
-    private final Pattern metadataComments = Pattern.compile("<!-- (?:(add|remove) contributor)|(?:summary: ')|(?:solves: ')|(?:Number of required Reviewers id marker)");
+    private final Pattern metadataComments = Pattern.compile("<!-- (?:(add|remove) contributor)|(?:summary: ')|(?:solves: ')|(?:additional required reviewers)");
     private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
 
     CheckWorkItem(PullRequest pr, HostedRepository censusRepo, String censusRef, Map<String, String> blockingLabels,

bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java

@@ -92,7 +92,8 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
             }
 
             var issues = prInstance.createVisitor(localHash, censusInstance);
-            prInstance.executeChecks(localHash, censusInstance, issues, AdditionalConfiguration.get(pr.repository().forge().currentUser(), allComments));
+            var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), allComments);
+            prInstance.executeChecks(localHash, censusInstance, issues, additionalConfiguration);
             if (!issues.getMessages().isEmpty()) {
                 reply.print("Your merge request cannot be fulfilled at this time, as ");
                 reply.println("your changes failed the final jcheck:");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java

@@ -106,7 +106,7 @@ public void visit(SelfReviewIssue e)
 
     @Override
     public void visit(TooFewReviewersIssue e) {
-        messages.add(String.format("Too few reviewers found (have %d, need at least %d)", e.numActual(), e.numRequired()));
+        messages.add(String.format("Too few reviewers with at least role %s found (have %d, need at least %d)", e.role(), e.numActual(), e.numRequired()));
         failedChecks.add(e.check().getClass());
     }
 

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java

@@ -27,30 +27,68 @@
 
 import java.io.PrintWriter;
 import java.nio.file.Path;
-import java.util.List;
+import java.util.*;
 
 public class ReviewersCommand implements CommandHandler {
+    private static final Map<String, String> roleMappings = Map.of(
+            "lead", "lead",
+            "reviewers", "reviewers",
+            "reviewer", "reviewers",
+            "committers", "committers",
+            "committer", "committers",
+            "authors", "authors",
+            "author", "author",
+            "contributors", "contributors",
+            "contributor", "contributors");
+
+    private void showHelp(PrintWriter reply) {
+        reply.println("Usage: `/reviewers <n> [<role>]` where `<n>` is the additional number of required reviewers. " +
+                              "If role is set, the reviewers need to have that project role. If omitted, role defaults to `committers`.");
+    }
+
     @Override
     public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
         if (!ProjectPermissions.mayReview(censusInstance, comment.author())) {
-            reply.println("Only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed to set the number of required Reviewers.");
+            reply.println("Only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed to increase the number of required reviewers.");
+            return;
+        }
+
+        var splitArgs = args.split(" ");
+        if (splitArgs.length < 1 || splitArgs.length > 2) {
+            showHelp(reply);
             return;
         }
 
         int numReviewers;
         try {
-            numReviewers = Integer.parseInt(args);
+            numReviewers = Integer.parseInt(splitArgs[0]);
         } catch (NumberFormatException e) {
-            reply.println("Usage: `/reviewers <n>` where `<n>` is the number of required Reviewers.");
+            showHelp(reply);
             return;
         }
 
-        reply.println(ReviewersTracker.setReviewersMarker(numReviewers));
-        reply.println("The number of required Reviewers is now set to " + numReviewers + ".");
+        if (numReviewers < 0 || numReviewers > 10) {
+            showHelp(reply);
+            reply.println("Number of additional required reviewers has to be between 0 and 10.");
+            return;
+        }
+
+        String role = "committers";
+        if (splitArgs.length > 1) {
+            if (!roleMappings.containsKey(splitArgs[1].toLowerCase())) {
+                showHelp(reply);
+                reply.println("Unknown role `" + splitArgs[1] + "` specified.");
+                return;
+            }
+            role = roleMappings.get(splitArgs[1].toLowerCase());
+        }
+
+        reply.println(ReviewersTracker.setReviewersMarker(numReviewers, role));
+        reply.println("The number of additional required reviews from " + role + " is now set to " + numReviewers + ".");
     }
 
     @Override
     public String description() {
-        return "set the number of required Reviewers for this PR";
+        return "set the number of additional required reviewers for this PR";
     }
 }

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersTracker.java

@@ -30,24 +30,27 @@
 import java.util.stream.Collectors;
 
 class ReviewersTracker {
-    private final static String reviewersMarker = "<!-- Number of required Reviewers id marker (%d) -->";
+    private final static String reviewersMarker = "<!-- additional required reviewers id marker (%d) (%s) -->";
     private final static Pattern reviewersMarkerPattern = Pattern.compile(
-            "<!-- Number of required Reviewers id marker \\((\\d+)\\) -->");
+            "<!-- additional required reviewers id marker \\((\\d+)\\) \\((\\w+)\\) -->");
 
-    static String setReviewersMarker(int numReviewers) {
-        return String.format(reviewersMarker, numReviewers);
+    static String setReviewersMarker(int numReviewers, String role) {
+        return String.format(reviewersMarker, numReviewers, role);
     }
 
-    static Optional<Integer> currentRequiredReviewers(HostUser botUser, List<Comment> comments) {
+    static Map<String, Integer> additionalRequiredReviewers(HostUser botUser, List<Comment> comments) {
+        var ret = new HashMap<String, Integer>();
         var reviewersActions = comments.stream()
                                        .filter(comment -> comment.author().equals(botUser))
                                        .map(comment -> reviewersMarkerPattern.matcher(comment.body()))
                                        .filter(Matcher::find)
                                        .collect(Collectors.toList());
         if (reviewersActions.isEmpty()) {
-            return Optional.empty();
+            return ret;
         }
-        var lastMatch = reviewersActions.get(reviewersActions.size() - 1);
-        return Optional.of(Integer.parseInt(lastMatch.group(1)));
+        for (var reviewersAction : reviewersActions) {
+            ret.put(reviewersAction.group(2), Integer.parseInt(reviewersAction.group(1)));
+        }
+        return ret;
     }
 }

bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java

@@ -88,7 +88,8 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
             }
 
             var issues = prInstance.createVisitor(localHash, censusInstance);
-            prInstance.executeChecks(localHash, censusInstance, issues, AdditionalConfiguration.get(pr.repository().forge().currentUser(), allComments));
+            var additionalConfiguration = AdditionalConfiguration.get(prInstance.localRepo(), localHash, pr.repository().forge().currentUser(), allComments);
+            prInstance.executeChecks(localHash, censusInstance, issues, additionalConfiguration);
             if (!issues.getMessages().isEmpty()) {
                 reply.print("Your merge request cannot be fulfilled at this time, as ");
                 reply.println("your changes failed the final jcheck:");

bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java

@@ -65,21 +65,36 @@ void simple(TestInfo testInfo) throws IOException {
             TestBotRunner.runPeriodicItems(prBot);
 
             // The bot should reply with a help message
-            assertLastCommentContains(reviewerPr,"is the number of required Reviewers");
+            assertLastCommentContains(reviewerPr,"is the additional number of required reviewers");
 
             // Invalid syntax
             reviewerPr.addComment("/reviewers two");
             TestBotRunner.runPeriodicItems(prBot);
 
             // The bot should reply with a help message
-            assertLastCommentContains(reviewerPr,"is the number of required Reviewers");
+            assertLastCommentContains(reviewerPr,"is the additional number of required reviewers");
+
+            // Too many
+            reviewerPr.addComment("/reviewers 7001");
+            TestBotRunner.runPeriodicItems(prBot);
+            assertLastCommentContains(reviewerPr,"Number of additional required reviewers has to be between");
+
+            // Too few
+            reviewerPr.addComment("/reviewers -3");
+            TestBotRunner.runPeriodicItems(prBot);
+            assertLastCommentContains(reviewerPr,"Number of additional required reviewers has to be between");
+
+            // Unknown role
+            reviewerPr.addComment("/reviewers 2 penguins");
+            TestBotRunner.runPeriodicItems(prBot);
+            assertLastCommentContains(reviewerPr,"Unknown role `penguins` specified");
 
             // Set the number
-            reviewerPr.addComment("/reviewers 2");
+            reviewerPr.addComment("/reviewers 1");
             TestBotRunner.runPeriodicItems(prBot);
 
             // The bot should reply with a success message
-            assertLastCommentContains(reviewerPr,"number of required Reviewers is now set to 2");
+            assertLastCommentContains(reviewerPr,"additional required reviews from committers is now set to 1");
 
             // Approve it as another user
             reviewerPr.addReview(Review.Verdict.APPROVED, "Approved");
@@ -91,13 +106,32 @@ void simple(TestInfo testInfo) throws IOException {
             assertFalse(updatedPr.labels().contains("ready"));
 
             // Now reduce the number of required reviewers
-            reviewerPr.addComment("/reviewers 1");
+            reviewerPr.addComment("/reviewers 0");
             TestBotRunner.runPeriodicItems(prBot);
             TestBotRunner.runPeriodicItems(prBot);
 
             // The PR should now be considered as ready for review
             updatedPr = author.pullRequest(pr.id());
             assertTrue(updatedPr.labels().contains("ready"));
+
+            // Now request that the lead reviews
+            reviewerPr.addComment("/reviewers 1 lead");
+            TestBotRunner.runPeriodicItems(prBot);
+            TestBotRunner.runPeriodicItems(prBot);
+            assertLastCommentContains(reviewerPr,"additional required reviews from lead is now set to 1");
+
+            // The PR should no longer be considered as ready for review
+            updatedPr = author.pullRequest(pr.id());
+            assertFalse(updatedPr.labels().contains("ready"));
+
+            // Drop the extra requirement
+            reviewerPr.addComment("/reviewers 0 lead");
+            TestBotRunner.runPeriodicItems(prBot);
+            TestBotRunner.runPeriodicItems(prBot);
+
+            // The PR should now be considered as ready for review yet again
+            updatedPr = author.pullRequest(pr.id());
+            assertTrue(updatedPr.labels().contains("ready"));
         }
     }
 
@@ -129,11 +163,11 @@ void noIntegration(TestInfo testInfo) throws IOException {
             var reviewerPr = integrator.pullRequest(pr.id());
 
             // Set the number
-            reviewerPr.addComment("/reviewers 2");
+            reviewerPr.addComment("/reviewers 1");
             TestBotRunner.runPeriodicItems(prBot);
 
             // The bot should reply with a success message
-            assertLastCommentContains(reviewerPr,"number of required Reviewers is now set to 2");
+            assertLastCommentContains(reviewerPr,"additional required reviews from committers is now set to 1");
 
             // Approve it as another user
             reviewerPr.addReview(Review.Verdict.APPROVED, "Approved");
@@ -143,10 +177,10 @@ 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 found (have 1, need at least 2)");
+            assertLastCommentContains(reviewerPr,"Too few reviewers with at least role committer found (have 0, need at least 1)");
 
             // Relax the requirement
-            reviewerPr.addComment("/reviewers 1");
+            reviewerPr.addComment("/reviewers 0");
             TestBotRunner.runPeriodicItems(prBot);
 
             // It should now work fine
@@ -194,19 +228,19 @@ void noSponsoring(TestInfo testInfo) throws IOException {
             assertLastCommentContains(reviewerPr,"now ready to be sponsored");
 
             // Set the number
-            reviewerPr.addComment("/reviewers 2");
+            reviewerPr.addComment("/reviewers 1");
             TestBotRunner.runPeriodicItems(prBot);
 
             // The bot should reply with a success message
-            assertLastCommentContains(reviewerPr,"number of required Reviewers is now set to 2");
+            assertLastCommentContains(reviewerPr,"additional required reviews from committers is now set to 1");
 
             // It should not be possible to sponsor
             reviewerPr.addComment("/sponsor");
             TestBotRunner.runPeriodicItems(prBot);
-            assertLastCommentContains(reviewerPr,"Too few reviewers found (have 1, need at least 2)");
+            assertLastCommentContains(reviewerPr,"Too few reviewers with at least role committer found (have 0, need at least 1)");
 
             // Relax the requirement
-            reviewerPr.addComment("/reviewers 1");
+            reviewerPr.addComment("/reviewers 0");
             TestBotRunner.runPeriodicItems(prBot);
 
             // It should now work fine

test/src/main/java/org/openjdk/skara/test/CheckableRepository.java

@@ -78,7 +78,7 @@ public static Repository init(Path path, VCS vcs, Path appendableFilePath, Set<S
             output.append("files=.*\\.txt\n");
             output.append("\n");
             output.append("[checks \"reviewers\"]\n");
-            output.append("minimum=1\n");
+            output.append("reviewers=1\n");
         }
         repo.add(checkConf);