Skip to content
Permalink
Browse files
330: Allow commands at end of PR body
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Jun 29, 2020
1 parent 0d45135 commit 95d655d5f2924f1aff1623cbeddd13458dd1b5d9
Showing with 384 additions and 137 deletions.
  1. +3 −3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/AllowCommand.java
  2. +4 −4 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java
  3. +1 −1 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java
  4. +8 −1 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandHandler.java
  5. +41 −0 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java
  6. +97 −23 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java
  7. +8 −3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/ContributorCommand.java
  8. +6 −6 bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java
  9. +9 −3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/IssueCommand.java
  10. +8 −3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java
  11. +0 −1 bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java
  12. +5 −5 bots/pr/src/main/java/org/openjdk/skara/bots/pr/RejectCommand.java
  13. +3 −3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java
  14. +3 −3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java
  15. +5 −5 bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java
  16. +27 −47 bots/pr/src/main/java/org/openjdk/skara/bots/pr/SummaryCommand.java
  17. +113 −2 bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java
  18. +17 −7 bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java
  19. +8 −2 bots/pr/src/test/java/org/openjdk/skara/bots/pr/SummaryTests.java
  20. +18 −15 test/src/main/java/org/openjdk/skara/test/TestBotRunner.java
@@ -31,11 +31,11 @@

public class AllowCommand implements CommandHandler {
@Override
public void handle(PullRequestBot bot, 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, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
var botUser = pr.repository().forge().currentUser();
var vetoers = Veto.vetoers(botUser, allComments);

if (!vetoers.contains(comment.author().id())) {
if (!vetoers.contains(command.user().id())) {
reply.println("You have not rejected this change");
return;
}
@@ -48,7 +48,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
reply.println("However, it still remains blocked by other rejections.");
}

reply.println(Veto.removeVeto(comment.author()));
reply.println(Veto.removeVeto(command.user()));
}

@Override
@@ -56,22 +56,22 @@ private static void linkReply(PullRequest pr, Issue issue, PrintWriter writer) {
}

@Override
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List<Comment> allComments, PrintWriter reply) {
if (!censusInstance.isReviewer(comment.author())) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
if (!censusInstance.isReviewer(command.user())) {
reply.println("only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed require a CSR.");
return;
}

var labels = pr.labels();

if (args.trim().toLowerCase().equals("unneeded")) {
if (command.args().trim().toLowerCase().equals("unneeded")) {
if (labels.contains(CSR_LABEL)) {
pr.removeLabel(CSR_LABEL);
}
reply.println("determined that a [CSR](https://wiki.openjdk.java.net/display/csr/Main) request " +
"is no longer needed for this pull request.");
return;
} else if (!args.isEmpty()) {
} else if (!command.args().isEmpty()) {
showHelp(reply);
return;
}
@@ -159,6 +159,6 @@ public Collection<WorkItem> run(Path scratchPath) {
throw new UncheckedIOException(e);
}
}
return List.of();
return List.of(new CommandWorkItem(bot, pr, errorHandler));
}
}
@@ -30,6 +30,13 @@
import java.util.List;

interface CommandHandler {
void handle(PullRequestBot bot, 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, CommandInvocation command, List<Comment> allComments, PrintWriter reply);
String description();

default boolean multiLine() {
return false;
}
default boolean allowedInBody() {
return false;
}
}
@@ -0,0 +1,41 @@
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.host.HostUser;

import java.util.Optional;

class CommandInvocation {
private final String id;
private final HostUser user;
private final CommandHandler handler;
private final String name;
private final String args;

CommandInvocation(String id, HostUser user, CommandHandler handler, String name, String args) {
this.id = id;
this.user = user;
this.handler = handler;
this.name = name;
this.args = args != null ? args.strip() : "";
}

String id() {
return id;
}

HostUser user() {
return user;
}

Optional<CommandHandler> handler() {
return Optional.ofNullable(handler);
}

String name() {
return name;
}

String args() {
return args;
}
}
@@ -24,6 +24,7 @@

import org.openjdk.skara.bot.WorkItem;
import org.openjdk.skara.forge.PullRequest;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.Comment;

import java.io.*;
@@ -37,7 +38,7 @@
public class CommandWorkItem extends PullRequestWorkItem {
private static final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");

private static final Pattern commandPattern = Pattern.compile("^/(.*)");
private static final Pattern commandPattern = Pattern.compile("^\\s*/([A-Za-z]+)(?:\\s+(.*))?");
private static final String commandReplyMarker = "<!-- Jmerge command reply message (%s) -->";
private static final Pattern commandReplyPattern = Pattern.compile("<!-- Jmerge command reply message \\((\\S+)\\) -->");
private static final String selfCommandMarker = "<!-- Valid self-command -->";
@@ -61,14 +62,14 @@ static class HelpCommand implements CommandHandler {
static private Map<String, String> external = null;

@Override
public void handle(PullRequestBot bot, 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, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
reply.println("Available commands:");
Stream.concat(
commandHandlers.entrySet().stream()
.map(entry -> entry.getKey() + " - " + entry.getValue().description()),
external.entrySet().stream()
.map(entry -> entry.getKey() + " - " + entry.getValue())
).sorted().forEachOrdered(command -> reply.println(" * " + command));
).sorted().forEachOrdered(c -> reply.println(" * " + c));
}

@Override
@@ -99,27 +100,102 @@ private List<AbstractMap.SimpleEntry<String, Comment>> findCommandComments(List<
.collect(Collectors.toList());
}

private void processCommand(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String command, Comment comment, List<Comment> allComments) {
private String formatId(String baseId, int subId) {
if (subId > 0) {
return String.format("%s:%d", baseId, subId);
} else {
return baseId;
}
}

private static class InvalidBodyCommandHandler implements CommandHandler {
@Override
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
reply.println("The command `" + command.name() + "` cannot be used in the pull request body. Please use it in a new comment.");
}

@Override
public String description() {
return "";
}
}

private List<CommandInvocation> extractCommands(String text, String baseId, HostUser user) {
var ret = new ArrayList<CommandInvocation>();
CommandHandler multiLineHandler = null;
List<String> multiLineBuffer = null;
String multiLineCommand = null;
int subId = 0;
for (var line : text.split("\\R")) {
var commandMatcher = commandPattern.matcher(line);
if (commandMatcher.matches()) {
if (multiLineHandler != null) {
ret.add(new CommandInvocation(formatId(baseId, subId++), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer)));
multiLineHandler = null;
}
var command = commandMatcher.group(1).toLowerCase();
var handler = commandHandlers.get(command);
if (handler != null && baseId.equals("body") && !handler.allowedInBody()) {
handler = new InvalidBodyCommandHandler();
}
if (handler != null && handler.multiLine()) {
multiLineHandler = handler;
multiLineBuffer = new ArrayList<>();
if (commandMatcher.group(2) != null) {
multiLineBuffer.add(commandMatcher.group(2));
}
multiLineCommand = command;
} else {
ret.add(new CommandInvocation(formatId(baseId, subId++), user, handler, command, commandMatcher.group(2)));
}
} else {
if (multiLineHandler != null) {
multiLineBuffer.add(line);
}
}
}
if (multiLineHandler != null) {
ret.add(new CommandInvocation(formatId(baseId, subId), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer)));
}
return ret;
}

private Optional<CommandInvocation> nextCommand(PullRequest pr, List<Comment> comments) {
var self = pr.repository().forge().currentUser();
var allCommands = Stream.concat(extractCommands(pr.body(), "body", pr.author()).stream(),
comments.stream()
.filter(comment -> !comment.author().equals(self) || comment.body().endsWith(selfCommandMarker))
.flatMap(c -> extractCommands(c.body(), c.id(), c.author()).stream()))
.collect(Collectors.toList());

var handled = comments.stream()
.filter(comment -> comment.author().equals(self))
.map(comment -> commandReplyPattern.matcher(comment.body()))
.filter(Matcher::find)
.map(matcher -> matcher.group(1))
.collect(Collectors.toSet());

return allCommands.stream()
.filter(ci -> !handled.contains(ci.id()))
.findFirst();
}

private void processCommand(PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments) {
var writer = new StringWriter();
var printer = new PrintWriter(writer);

printer.println(String.format(commandReplyMarker, comment.id()));
printer.println(String.format(commandReplyMarker, command.id()));
printer.print("@");
printer.print(comment.author().userName());
printer.print(command.user().userName());
printer.print(" ");

command = command.strip();
var argSplit = command.indexOf(' ');
var commandWord = argSplit > 0 ? command.substring(0, argSplit) : command;
var commandArgs = argSplit > 0 ? command.substring(argSplit + 1) : "";

var handler = commandHandlers.get(commandWord);
if (handler != null) {
handler.handle(bot, pr, censusInstance, scratchPath, commandArgs, comment, allComments, printer);
var handler = command.handler();
if (handler.isPresent()) {
handler.get().handle(bot, pr, censusInstance, scratchPath, command, allComments, printer);
} else {
if (!bot.externalCommands().containsKey(commandWord)) {
if (!bot.externalCommands().containsKey(command.name())) {
printer.print("Unknown command `");
printer.print(command);
printer.print(command.name());
printer.println("` - for a list of valid commands use `/help`.");
} else {
// Do not reply to external commands
@@ -132,17 +208,17 @@ private void processCommand(PullRequest pr, CensusInstance censusInstance, Path

@Override
public Collection<WorkItem> run(Path scratchPath) {
log.info("Looking for merge commands");
log.info("Looking for PR commands");

if (pr.labels().contains("integrated")) {
log.info("Skip checking for commands in integrated PR");
return List.of();
}

var comments = pr.comments();
var unprocessedCommands = findCommandComments(comments);
if (unprocessedCommands.isEmpty()) {
log.fine("No new merge commands found, stopping further processing");
var nextCommand = nextCommand(pr, comments);
if (nextCommand.isEmpty()) {
log.fine("No new PR commands found, stopping further processing");
return List.of();
}

@@ -151,9 +227,7 @@ public Collection<WorkItem> run(Path scratchPath) {
}

var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);
for (var entry : unprocessedCommands) {
processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), entry.getKey(), entry.getValue(), comments);
}
processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), nextCommand.get(), comments);

// Run another check to reflect potential changes from commands
return List.of(new CheckWorkItem(bot, pr, errorHandler));
@@ -79,13 +79,13 @@ private Optional<EmailAddress> parseUser(String user, PullRequest pr, CensusInst
}

@Override
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())) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
if (!command.user().equals(pr.author())) {
reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `contributor` command.");
return;
}

var matcher = commandPattern.matcher(args);
var matcher = commandPattern.matcher(command.args());
if (!matcher.matches()) {
showHelp(pr, reply);
return;
@@ -130,4 +130,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
public String description() {
return "adds or removes additional contributors for a PR";
}

@Override
public boolean allowedInBody() {
return true;
}
}
@@ -53,14 +53,14 @@ private Optional<String> checkProblem(Map<String, Check> performedChecks, String
}

@Override
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())) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
if (!command.user().equals(pr.author())) {
reply.print("Only the author (@" + pr.author().userName() + ") is allowed to issue the `integrate` command.");

// If the command author is allowed to sponsor this change, suggest that command
var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments);
if (readyHash.isPresent()) {
if (censusInstance.isCommitter(comment.author())) {
if (censusInstance.isCommitter(command.user())) {
reply.print(" As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the `/sponsor` command?");
return;
}
@@ -94,8 +94,8 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

// Validate the target hash if requested
var rebaseMessage = new StringWriter();
if (!args.isBlank()) {
var wantedHash = new Hash(args);
if (!command.args().isBlank()) {
var wantedHash = new Hash(command.args());
if (!pr.targetHash().equals(wantedHash)) {
reply.print("The head of the target branch is no longer at the requested hash " + wantedHash);
reply.println(" - it has moved to " + pr.targetHash() + ". Aborting integration.");
@@ -129,7 +129,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
if (!censusInstance.isCommitter(pr.author())) {
reply.println(ReadyForSponsorTracker.addIntegrationMarker(pr.headHash()));
reply.println("Your change (at version " + pr.headHash() + ") is now ready to be sponsored by a Committer.");
if (!args.isBlank()) {
if (!command.args().isBlank()) {
reply.println("Note that your sponsor will make the final decision onto which target hash to integrate.");
}
pr.addLabel("sponsor");
@@ -272,11 +272,12 @@ private void createIssue(PullRequestBot bot, PullRequest pr, String args, Census
}

@Override
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())) {
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
if (!command.user().equals(pr.author())) {
reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `/" + name + "` command.");
return;
}
var args = command.args();
if (args.isBlank()) {
showHelp(reply);
return;
@@ -295,7 +296,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
if (args.startsWith("remove") || args.startsWith("delete")) {
removeIssue(bot, args, currentSolved, reply);
} else if (args.startsWith("create")) {
createIssue(bot, pr, args, censusInstance, comment.author(), reply);
createIssue(bot, pr, args, censusInstance, command.user(), reply);
} else {
addIssue(bot, pr, args, currentSolved, reply);
}
@@ -308,4 +309,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
public String description() {
return "edit the list of issues that this PR solves";
}

@Override
public boolean allowedInBody() {
return true;
}
}

1 comment on commit 95d655d

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented on 95d655d Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.