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

Improve bot message formatting #799

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -24,6 +24,7 @@

import org.openjdk.skara.bot.WorkItem;
import org.openjdk.skara.forge.PullRequest;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.json.*;
import org.openjdk.skara.storage.StorageBuilder;
import org.openjdk.skara.vcs.Hash;
@@ -32,7 +33,7 @@
import java.nio.file.Path;
import java.util.*;
import java.util.function.Consumer;
import java.util.regex.Pattern;
import java.util.regex.*;
import java.util.stream.*;

public class PullRequestWorkItem implements WorkItem {
@@ -50,21 +51,21 @@
this.integratorId = integratorId;
}

private Hash resultingCommitHashFor(PullRequest pr) {
if (pr.labels().contains("integrated")) {
for (var comment : pr.comments()) {
if (comment.author().id().equals(integratorId)) {
for (var line : comment.body().split("\n")) {
if (line.startsWith("Pushed as commit")) {
var parts = line.split(" ");
var hash = parts[parts.length - 1].replace(".", "");
return new Hash(hash);
}
}
}
}
}
return null;
private final static Pattern pushedPattern = Pattern.compile("Pushed as commit ([a-f0-9]{40})\\.");

private Hash resultingCommitHash() {
if (pr.labels().contains("integrated")) {
return pr.comments().stream()
.filter(comment -> comment.author().id().equals(integratorId))
.map(Comment::body)
.map(pushedPattern::matcher)
.filter(Matcher::find)
.map(m -> m.group(1))
.map(Hash::new)
.findAny()
.orElse(null);
}
return null;
}

private Set<PullRequestState> deserializePrState(String current) {
@@ -179,7 +180,7 @@ private void notifyIntegratedPr(PullRequest pr, Hash hash) {
.materialize(historyPath);

var issues = parseIssues();
var commit = resultingCommitHashFor(pr);
var commit = resultingCommitHash();
var state = new PullRequestState(pr, issues, commit);
var stored = storage.current();
if (stored.contains(state)) {
@@ -193,7 +194,7 @@ private void notifyIntegratedPr(PullRequest pr, Hash hash) {
.findAny();
// The stored entry could be old and be missing commit information - if so, upgrade it
if (storedState.isPresent() && storedState.get().commitId().equals(Optional.of(Hash.zero()))) {
var hash = resultingCommitHashFor(pr);
var hash = resultingCommitHash();
storedState = Optional.of(new PullRequestState(pr, storedState.get().issueIds(), hash));
storage.put(storedState.get());
}
@@ -23,12 +23,11 @@
package org.openjdk.skara.bots.notify.comment;

import org.junit.jupiter.api.*;
import org.openjdk.skara.bots.notify.*;
import org.openjdk.skara.bots.notify.NotifyBot;
import org.openjdk.skara.json.JSON;
import org.openjdk.skara.test.*;

import java.io.IOException;
import java.net.URI;
import java.util.*;
import java.util.regex.Pattern;

@@ -76,7 +75,7 @@ void testCommitComment(TestInfo testInfo) throws IOException {
var pr = credentials.createPullRequest(repo, "master", "master", "Fix an issue");
pr.setBody("I made a fix");
pr.addLabel("integrated");
pr.addComment("Pushed as commit " + editHash.hex() + ".");
pr.addComment("More text!\n\n@user Pushed as commit " + editHash.hex() + ". Even more text.\n\nAnd some additional text.");
TestBotRunner.runPeriodicItems(notifyBot);

// Check commit comment
@@ -155,7 +155,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
if (!finalRebaseMessage.isBlank()) {
reply.println(rebaseMessage.toString());
}
reply.println("Pushed as commit " + localHash.hex() + ". :bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
reply.println("Pushed as commit " + localHash.hex() + ".");
reply.println();
reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
localRepo.push(localHash, pr.repository().url(), pr.targetRef());
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
@@ -73,25 +73,28 @@ private void updateLabelMessage(List<Comment> comments, List<String> newLabels)
if (newLabels.isEmpty()) {
message.append("To determine the appropriate audience for reviewing this pull request, one or more ");
message.append("labels corresponding to different subsystems will normally be applied automatically. ");
message.append("However, no automatic labelling rule matches the changes in this pull request. ");
message.append("However, no automatic labelling rule matches the changes in this pull request.\n\n");
message.append("In order to have an RFR email automatically sent to the correct mailing list, you will ");
message.append("need to add one or more labels manually using the `/label add \"label\"` command. ");
message.append("The following labels are valid: ");
var labels = bot.labelConfiguration().allowed().stream()
.sorted()
.map(label -> "`" + label + "`")
.collect(Collectors.joining(", "));
.collect(Collectors.joining(" "));
message.append(labels);
message.append(".");
} else {
message.append("The following labels will be automatically applied to this pull request: ");
message.append("The following label");
if (newLabels.size() > 1) {
message.append("s");
}
message.append(" will be automatically applied to this pull request: ");
var labels = newLabels.stream()
.sorted()
.map(label -> "`" + label + "`")
.collect(Collectors.joining(", "));
message.append("`");
.collect(Collectors.joining(" "));
message.append(labels);
message.append("`. When this pull request is ready to be reviewed, an RFR email will be sent to the ");
message.append(".\n\nWhen this pull request is ready to be reviewed, an RFR email will be sent to the ");
message.append("corresponding mailing list");
if (newLabels.size() > 1) {
message.append("s");
@@ -129,7 +129,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
if (!finalRebaseMessage.isBlank()) {
reply.println(rebaseMessage.toString());
}
reply.println("Pushed as commit " + localHash.hex() + ". :bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
reply.println("Pushed as commit " + localHash.hex() + ".");
reply.println();
reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
localRepo.push(localHash, pr.repository().url(), pr.targetRef());
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
@@ -163,7 +163,7 @@ void adjustAutoApplied(TestInfo testInfo) throws IOException {
// The bot should have applied one label automatically
TestBotRunner.runPeriodicItems(prBot);
assertEquals(Set.of("2", "rfr"), new HashSet<>(pr.labels()));
assertLastCommentContains(pr, "The following labels will be automatically applied");
assertLastCommentContains(pr, "The following label will be automatically applied");
assertLastCommentContains(pr, "`2`");

// It will refuse to remove it
@@ -71,7 +71,7 @@ void simple(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(labelBot);
assertEquals(Set.of("rfr"), new HashSet<>(pr.labels()));
assertLastCommentContains(pr, "However, no automatic labelling rule matches the changes in this pull request.");
assertLastCommentContains(pr, "The following labels are valid: `test1`, `test2`");
assertLastCommentContains(pr, "The following labels are valid: `test1` `test2`");

var fileA = localRepoFolder.resolve("a.txt");
Files.writeString(fileA, "Hello");
ProTip! Use n and p to navigate between commits in a pull request.