Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

217: Enforce check on number of required reviewers #364

Closed
wants to merge 5 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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;
}
@@ -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 {
@@ -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,
@@ -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:");
@@ -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());
}

@@ -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";
}
}
@@ -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;
}
}
@@ -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:");
@@ -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
@@ -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);