RFR: 394: Merge bot conflict resolution interaction requires separate credentials

Jorn Vernee jvernee at openjdk.java.net
Fri May 8 16:07:12 UTC 2020

On Fri, 8 May 2020 14:32:57 GMT, Robin Westberg <rwestberg at openjdk.org> wrote:

> Hi all,
> Please review this change that allows the pull request bot to recognize specially formatted command comments made by
> "itself" (most likely another bot using the same credentials).
> Best regards,
> Robin

Looks good. Some minor comments left inline. The one about the test bot running early in the test might be the only
problematic thing (but I don't think it matters in practice).

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java line 42:

> 41:     private final Pattern commandReplyPattern = Pattern.compile("<!-- Jmerge command reply message
> \\((\\S+)\\) -->"); 42:     private final String selfCommandMarker = "<!-- Valid self-command -->";
> 43:

FWIW, these fields could also be made `static` as well.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java line 91:

> 90:                        .map(comment -> new AbstractMap.SimpleEntry<>(comment,
> commandPattern.matcher(comment.body()))) 91:                        .filter(entry ->
> !entry.getKey().author().equals(self) || entry.getKey().body().endsWith(selfCommandMarker)) 92:
> .filter(entry -> entry.getValue().find())

Could you move this above the `.map(...)` call, so that you can replace `entry.getKey()` with `comment`? (just like

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java line 137:

> 136:             TestBotRunner.runPeriodicItems(mergeBot);
> 137:             assertEquals(1, pr.comments().size());
> 138:

Should to `runPeriodicItems` call right after adding the "/help" comment be there? Asking because it looks like you
want to check the number of comments before and after running the bot below that (but at that point the bot has already

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java line 149:

> 148:                          .filter(comment -> comment.body().contains("help"))
> 149:                           .count();
> 150:             assertEquals(1, help);

Spurious whitespace on last line

            var help = pr.comments().stream()
                         .filter(comment -> comment.body().contains("Available commands"))
                         .filter(comment -> comment.body().contains("help"))


Marked as reviewed by jvernee (Reviewer).

PR: https://git.openjdk.java.net/skara/pull/618

More information about the skara-dev mailing list