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

223: Make required re-reviewing configurable #376

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
@@ -31,7 +31,7 @@

public class AllowCommand implements CommandHandler {
@Override
public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
var botUser = pr.repository().forge().currentUser();
var vetoers = Veto.vetoers(botUser, allComments);

@@ -187,12 +187,6 @@ private void updateCheckBuilder(CheckBuilder checkBuilder, PullRequestCheckIssue
}

private void updateReadyForReview(PullRequestCheckIssueVisitor visitor, List<String> additionalErrors) {
// If there are no issues at all, the PR is already reviewed
if (visitor.getMessages().isEmpty() && additionalErrors.isEmpty()) {
pr.removeLabel("rfr");
return;
}

// Additional errors are not allowed
if (!additionalErrors.isEmpty()) {
newLabels.remove("rfr");
@@ -39,21 +39,11 @@
import java.util.stream.Collectors;

class CheckWorkItem extends PullRequestWorkItem {
private final HostedRepository censusRepo;
private final String censusRef;
private final Map<String, String> blockingLabels;
private final IssueProject issueProject;

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,
Consumer<RuntimeException> errorHandler, IssueProject issueProject) {
super(pr, errorHandler);
this.censusRepo = censusRepo;
this.censusRef = censusRef;
this.blockingLabels = blockingLabels;
this.issueProject = issueProject;
CheckWorkItem(PullRequestBot bot, PullRequest pr, Consumer<RuntimeException> errorHandler) {
super(bot, pr, errorHandler);
}

private String encodeReviewer(HostUser reviewer, CensusInstance censusInstance) {
@@ -144,7 +134,7 @@ public String toString() {
@Override
public void run(Path scratchPath) {
// First determine if the current state of the PR has already been checked
var census = CensusInstance.create(censusRepo, censusRef, scratchPath.resolve("census"), pr);
var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);
var comments = pr.comments();
var allReviews = pr.reviews();
var labels = new HashSet<>(pr.labels());
@@ -158,9 +148,9 @@ public void run(Path scratchPath) {
}

try {
var prInstance = new PullRequestInstance(scratchPath.resolve("pr"), pr);
var prInstance = new PullRequestInstance(scratchPath.resolve("pr"), pr, bot.ignoreStaleReviews());
CheckRun.execute(this, pr, prInstance, comments, allReviews, activeReviews, labels, census,
blockingLabels, issueProject);
bot.blockingLabels(), bot.issueProject());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
@@ -30,6 +30,6 @@
import java.util.List;

interface CommandHandler {
void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply);
void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply);
String description();
}
@@ -34,10 +34,6 @@
import java.util.stream.*;

public class CommandWorkItem extends PullRequestWorkItem {
private final HostedRepository censusRepo;
private final String censusRef;
private final Map<String, String> external;

private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");

private final String commandReplyMarker = "<!-- Jmerge command reply message (%s) -->";
@@ -57,7 +53,7 @@ static class HelpCommand implements CommandHandler {
static private Map<String, String> external = null;

@Override
public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
reply.println("Available commands:");
Stream.concat(
commandHandlers.entrySet().stream()
@@ -73,15 +69,8 @@ public String description() {
}
}

CommandWorkItem(PullRequest pr, HostedRepository censusRepo, String censusRef, Map<String, String> external, Consumer<RuntimeException> errorHandler) {
super(pr, errorHandler);
this.censusRepo = censusRepo;
this.censusRef = censusRef;
this.external = external;

if (HelpCommand.external == null) {
HelpCommand.external = external;
}
CommandWorkItem(PullRequestBot bot, PullRequest pr, Consumer<RuntimeException> errorHandler) {
super(bot, pr, errorHandler);
}

private List<AbstractMap.SimpleEntry<String, Comment>> findCommandComments(List<Comment> comments) {
@@ -120,9 +109,9 @@ private void processCommand(PullRequest pr, CensusInstance censusInstance, Path

var handler = commandHandlers.get(commandWord);
if (handler != null) {
handler.handle(pr, censusInstance, scratchPath, commandArgs, comment, allComments, printer);
handler.handle(bot, pr, censusInstance, scratchPath, commandArgs, comment, allComments, printer);
} else {
if (!external.containsKey(commandWord)) {
if (!bot.externalCommands().containsKey(commandWord)) {
printer.print("Unknown command `");
printer.print(command);
printer.println("` - for a list of valid commands use `/help`.");
@@ -151,7 +140,11 @@ public void run(Path scratchPath) {
return;
}

var census = CensusInstance.create(censusRepo, censusRef, scratchPath.resolve("census"), pr);
if (HelpCommand.external == null) {
HelpCommand.external = bot.externalCommands();
}

var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);
for (var entry : unprocessedCommands) {
processCommand(pr, census, scratchPath.resolve("pr"), entry.getKey(), entry.getValue(), comments);
}
@@ -35,7 +35,7 @@ public class ContributorCommand implements CommandHandler {
private static final Pattern commandPattern = Pattern.compile("^(add|remove)\\s+(.*?\\s+<\\S+>)$");

@Override
public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
if (!comment.author().equals(pr.author())) {
reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `contributor` command.");
return;
@@ -54,7 +54,7 @@ private Optional<String> checkProblem(Map<String, Check> performedChecks, String
}

@Override
public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
if (!comment.author().equals(pr.author())) {
reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `integrate` command.");
return;
@@ -77,7 +77,7 @@ public void handle(PullRequest pr, CensusInstance censusInstance, Path scratchPa
var sanitizedUrl = URLEncoder.encode(pr.repository().webUrl().toString(), StandardCharsets.UTF_8);
var path = scratchPath.resolve("pr.integrate").resolve(sanitizedUrl);

var prInstance = new PullRequestInstance(path, pr);
var prInstance = new PullRequestInstance(path, pr, bot.ignoreStaleReviews());
var localHash = prInstance.commit(censusInstance.namespace(), censusInstance.configuration().census().domain(), null);
var rebaseMessage = new StringWriter();
var rebaseWriter = new PrintWriter(rebaseMessage);
@@ -23,24 +23,16 @@
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.forge.PullRequest;
import org.openjdk.skara.vcs.Hash;

import java.io.*;
import java.nio.file.Path;
import java.util.*;
import java.util.concurrent.ConcurrentMap;
import java.util.function.Consumer;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

public class LabelerWorkItem extends PullRequestWorkItem {
private final Map<String, List<Pattern>> labelPatterns;
private final ConcurrentMap<Hash, Boolean> currentLabels;

LabelerWorkItem(PullRequest pr, Map<String, List<Pattern>> labelPatterns, ConcurrentMap<Hash, Boolean> currentLabels, Consumer<RuntimeException> errorHandler) {
super(pr, errorHandler);
this.labelPatterns = labelPatterns;
this.currentLabels = currentLabels;
LabelerWorkItem(PullRequestBot bot, PullRequest pr, Consumer<RuntimeException> errorHandler) {
super(bot, pr, errorHandler);
}

@Override
@@ -52,7 +44,7 @@ private Set<String> getLabels(PullRequestInstance prInstance) throws IOException
var labels = new HashSet<String>();
var files = prInstance.changedFiles();
for (var file : files) {
for (var label : labelPatterns.entrySet()) {
for (var label : bot.labelPatterns().entrySet()) {
for (var pattern : label.getValue()) {
var matcher = pattern.matcher(file.toString());
if (matcher.find()) {
@@ -67,14 +59,14 @@ private Set<String> getLabels(PullRequestInstance prInstance) throws IOException

@Override
public void run(Path scratchPath) {
if (currentLabels.containsKey(pr.headHash())) {
if (bot.currentLabels().containsKey(pr.headHash())) {
return;
}
try {
var prInstance = new PullRequestInstance(scratchPath.resolve("labeler"), pr);
var prInstance = new PullRequestInstance(scratchPath.resolve("labeler"), pr, bot.ignoreStaleReviews());
var newLabels = getLabels(prInstance);
var currentLabels = pr.labels().stream()
.filter(labelPatterns::containsKey)
.filter(key -> bot.labelPatterns().containsKey(key))
.collect(Collectors.toSet());

// Add all labels not already set
@@ -87,7 +79,7 @@ public void run(Path scratchPath) {
.filter(label -> !newLabels.contains(label))
.forEach(pr::removeLabel);

this.currentLabels.put(pr.headHash(), Boolean.TRUE);
bot.currentLabels().put(pr.headHash(), Boolean.TRUE);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
@@ -43,14 +43,15 @@ class PullRequestBot implements Bot {
private final Set<String> readyLabels;
private final Map<String, Pattern> readyComments;
private final IssueProject issueProject;
private final ConcurrentMap<Hash, Boolean> currentLabels = new ConcurrentHashMap<>();
private final boolean ignoreStaleReviews;
private final ConcurrentMap<Hash, Boolean> currentLabels;
private final PullRequestUpdateCache updateCache;
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");

PullRequestBot(HostedRepository repo, HostedRepository censusRepo, String censusRef,
Map<String, List<Pattern>> labelPatterns, Map<String, String> externalCommands,
Map<String, String> blockingLabels, Set<String> readyLabels,
Map<String, Pattern> readyComments, IssueProject issueProject) {
Map<String, Pattern> readyComments, IssueProject issueProject, boolean ignoreStaleReviews) {
remoteRepo = repo;
this.censusRepo = censusRepo;
this.censusRef = censusRef;
@@ -60,18 +61,14 @@ class PullRequestBot implements Bot {
this.readyLabels = readyLabels;
this.issueProject = issueProject;
this.readyComments = readyComments;
this.updateCache = new PullRequestUpdateCache();
}
this.ignoreStaleReviews = ignoreStaleReviews;

PullRequestBot(HostedRepository repo, HostedRepository censusRepo, String censusRef,
Map<String, List<Pattern>> labelPatterns, Map<String, String> externalCommands,
Map<String, String> blockingLabels, Set<String> readyLabels,
Map<String, Pattern> readyComments) {
this(repo, censusRepo, censusRef, labelPatterns, externalCommands, blockingLabels, readyLabels, readyComments, null);
this.currentLabels = new ConcurrentHashMap<>();
this.updateCache = new PullRequestUpdateCache();
}

PullRequestBot(HostedRepository repo, HostedRepository censusRepo, String censusRef) {
this(repo, censusRepo, censusRef, Map.of(), Map.of(), Map.of(), Set.of(), Map.of(), null);
static PullRequestBotBuilder newBuilder() {
return new PullRequestBotBuilder();
}

private boolean isReady(PullRequest pr) {
@@ -113,9 +110,9 @@ private List<WorkItem> getWorkItems(List<PullRequest> pullRequests) {
continue;
}

ret.add(new CheckWorkItem(pr, censusRepo, censusRef, blockingLabels, e -> updateCache.invalidate(pr), issueProject));
ret.add(new CommandWorkItem(pr, censusRepo, censusRef, externalCommands, e -> updateCache.invalidate(pr)));
ret.add(new LabelerWorkItem(pr, labelPatterns, currentLabels, e -> updateCache.invalidate(pr)));
ret.add(new CheckWorkItem(this, pr, e -> updateCache.invalidate(pr)));
ret.add(new CommandWorkItem(this, pr, e -> updateCache.invalidate(pr)));
ret.add(new LabelerWorkItem(this, pr, e -> updateCache.invalidate(pr)));
}
}

@@ -136,4 +133,44 @@ public List<WorkItem> processWebHook(JSONValue body) {

return getWorkItems(webHook.get().updatedPullRequests());
}

HostedRepository censusRepo() {
return censusRepo;
}

String censusRef() {
return censusRef;
}

Map<String, List<Pattern>> labelPatterns() {
return labelPatterns;
}

Map<String, String> externalCommands() {
return externalCommands;
}

Map<String, String> blockingLabels() {
return blockingLabels;
}

Set<String> readyLabels() {
return readyLabels;
}

Map<String, Pattern> readyComments() {
return readyComments;
}

IssueProject issueProject() {
return issueProject;
}

ConcurrentMap<Hash, Boolean> currentLabels() {
return currentLabels;
}

boolean ignoreStaleReviews() {
return ignoreStaleReviews;
}
}