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 all 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
@@ -0,0 +1,48 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.pr;

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(ReadOnlyRepository repository, Hash hash, HostUser botUser, List<Comment> comments) throws IOException {
var ret = new ArrayList<String>();
var additionalReviewers = ReviewersTracker.additionalRequiredReviewers(botUser, comments);
if (additionalReviewers.isEmpty()) {
return ret;
}

var currentConfiguration = JCheckConfiguration.from(repository, hash);
var updatedLimits = ReviewersTracker.updatedRoleLimits(currentConfiguration, additionalReviewers.get().number(), additionalReviewers.get().role());

ret.add("[checks \"reviewers\"]");
updatedLimits.forEach((role, count) -> ret.add(role + "=" + count));
return ret;
}
}
@@ -482,7 +482,8 @@ private void checkStatus() {
}

// Determine current status
prInstance.executeChecks(localHash, censusInstance, visitor);
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: ')");
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,
@@ -49,7 +49,8 @@ public class CommandWorkItem extends PullRequestWorkItem {
"sponsor", new SponsorCommand(),
"contributor", new ContributorCommand(),
"summary", new SummaryCommand(),
"solves", new SolvesCommand()
"solves", new SolvesCommand(),
"reviewers", new ReviewersCommand()
);

static class HelpCommand implements CommandHandler {
@@ -92,7 +92,8 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
}

var issues = prInstance.createVisitor(localHash, censusInstance);
prInstance.executeChecks(localHash, censusInstance, issues);
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());
}

@@ -25,7 +25,7 @@
import org.openjdk.skara.census.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.*;
import org.openjdk.skara.jcheck.JCheck;
import org.openjdk.skara.jcheck.*;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.Issue;
import org.openjdk.skara.vcs.openjdk.*;
@@ -215,9 +215,9 @@ PullRequestCheckIssueVisitor createVisitor(Hash localHash, CensusInstance census
return new PullRequestCheckIssueVisitor(checks);
}

void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor) throws Exception {
void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor, List<String> additionalConfiguration) throws Exception {
try (var issues = JCheck.check(localRepo(), censusInstance.census(), CommitMessageParsers.v1, "HEAD~1..HEAD",
localHash, new HashMap<>(), new HashSet<>())) {
localHash, new HashMap<>(), new HashSet<>(), additionalConfiguration)) {
for (var issue : issues) {
issue.accept(visitor);
}
@@ -0,0 +1,114 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.forge.PullRequest;
import org.openjdk.skara.issuetracker.Comment;

import java.io.PrintWriter;
import java.nio.file.Path;
import java.util.*;
import java.util.stream.Collectors;

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 number of required reviewers. " +
"If role is set, the reviewers need to have that project role. If omitted, role defaults to `authors`.");
}

@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 change 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(splitArgs[0]);
} catch (NumberFormatException e) {
showHelp(reply);
return;
}
if (numReviewers > 10) {
showHelp(reply);
reply.println("Cannot increase the required number of reviewers above 10 (requested: " + numReviewers + ")");
return;
}

String role = "authors";
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());
}

var updatedLimits = ReviewersTracker.updatedRoleLimits(censusInstance.configuration(), numReviewers, role);
if (updatedLimits.get(role) > numReviewers) {
showHelp(reply);
reply.println("Number of required reviewers of role " + role + " cannot be decreased below " + updatedLimits.get(role));
return;
}

reply.println(ReviewersTracker.setReviewersMarker(numReviewers, role));
var totalRequired = updatedLimits.values().stream().mapToInt(Integer::intValue).sum();
reply.print("The number of required reviews for this PR is now set to " + totalRequired);

// Create a helpful message regarding the required distribution (if applicable)
var nonZeroDescriptions = updatedLimits.entrySet().stream()
.filter(entry -> entry.getValue() > 0)
.map(entry -> entry.getValue() + " of role " + entry.getKey())
.collect(Collectors.toList());
if (nonZeroDescriptions.size() > 1) {
nonZeroDescriptions.remove(nonZeroDescriptions.size() - 1);
reply.print(" (with at least " + String.join(", ", nonZeroDescriptions) + ")");
}

reply.println(".");
}

@Override
public String description() {
return "set the number of additional required reviewers for this PR";
}
}
@@ -0,0 +1,114 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.jcheck.JCheckConfiguration;

import java.util.*;
import java.util.regex.*;
import java.util.stream.Collectors;

class ReviewersTracker {
private final static String reviewersMarker = "<!-- additional required reviewers id marker (%d) (%s) -->";
private final static Pattern reviewersMarkerPattern = Pattern.compile(
"<!-- additional required reviewers id marker \\((\\d+)\\) \\((\\w+)\\) -->");

static String setReviewersMarker(int numReviewers, String role) {
return String.format(reviewersMarker, numReviewers, role);
}

static LinkedHashMap<String, Integer> updatedRoleLimits(JCheckConfiguration checkConfiguration, int count, String role) {
var currentReviewers = checkConfiguration.checks().reviewers();

var updatedLimits = new LinkedHashMap<String, Integer>();
updatedLimits.put("lead", currentReviewers.lead());
updatedLimits.put("reviewers", currentReviewers.reviewers());
updatedLimits.put("committers", currentReviewers.committers());
updatedLimits.put("authors", currentReviewers.authors());
updatedLimits.put("contributors", currentReviewers.contributors());

// Increase the required role level by the requested amount (while subtracting higher roles)
var remainingAdditional = count;
var roles = new ArrayList<>(updatedLimits.keySet());
for (var r : roles) {
if (!r.equals(role)) {
remainingAdditional -= updatedLimits.get(r);
if (remainingAdditional <= 0) {
break;
}
} else {
updatedLimits.replace(r, updatedLimits.get(r) + remainingAdditional);
break;
}
}

// Decrease lower roles (if any) to avoid increasing the total count above the requested
Collections.reverse(roles);
var remainingRemovals = count;
for (var r : roles) {
if (!r.equals(role)) {
var removed = Math.max(0, updatedLimits.get(r) - remainingRemovals);
updatedLimits.replace(r, removed);
remainingRemovals -= removed;
} else {
break;
}
}

return updatedLimits;
}

static class AdditionalRequiredReviewers {
private int number;
private String role;

AdditionalRequiredReviewers(int number, String role) {
this.number = number;
this.role = role;
}

int number() {
return number;
}

String role() {
return role;
}
}

static Optional<AdditionalRequiredReviewers> 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();
}
var last = reviewersActions.get(reviewersActions.size() - 1);
return Optional.of(new AdditionalRequiredReviewers(Integer.parseInt(last.group(1)), last.group(2)));
}
}
@@ -88,7 +88,8 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
}

var issues = prInstance.createVisitor(localHash, censusInstance);
prInstance.executeChecks(localHash, censusInstance, issues);
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:");