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

330: Allow commands at end of PR body #682

Closed
wants to merge 7 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,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;
}
}