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.

Sign up for GitHub

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

Jump to bottom

963: Skara mlbridge bot sometimes resends old messages #1117

Closed
wants to merge 8 commits into from
Closed

963: Skara mlbridge bot sometimes resends old messages #1117

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3d309a3
Improve mbox parsing
Mar 26, 2021
c725c04
Add support for treating a set of mboxes as a single source of conver…
Mar 30, 2021
ea48992
Fix configuration parsing
Mar 31, 2021
2977dd0
Rename MailingList -> MailingListReader
Mar 31, 2021
e397e79
Merge remote-tracking branch 'upstream/master' into mbox_crosspost
Apr 14, 2021
4b16916
Fix build error after merge
Apr 14, 2021
5c6b496
Do not try to parse sent mails as a conversation as we just want the …
Apr 14, 2021
ea76253
Merge remote-tracking branch 'upstream/master' into mlbridge_archive_…
Apr 14, 2021
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
6 changes: 3 additions & 3 deletions bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveReaderWorkItem.java
View file
Open in desktop
Expand Up @@ -23,17 +23,17 @@
package org.openjdk.skara.bots.mlbridge;

import org.openjdk.skara.bot.WorkItem;
import org.openjdk.skara.mailinglist.MailingList;
import org.openjdk.skara.mailinglist.MailingListReader;

import java.nio.file.Path;
import java.time.Duration;
import java.util.*;

public class ArchiveReaderWorkItem implements WorkItem {
private final MailingListArchiveReaderBot bot;
private final MailingList list;
private final MailingListReader list;

ArchiveReaderWorkItem(MailingListArchiveReaderBot bot, MailingList list) {
ArchiveReaderWorkItem(MailingListArchiveReaderBot bot, MailingListReader list) {
this.bot = bot;
this.list = list;
}
Expand Down
39 changes: 18 additions & 21 deletions bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java
View file
Open in desktop
Expand Up @@ -31,7 +31,8 @@
import org.openjdk.skara.vcs.*;

import java.io.*;
import java.nio.file.Path;
import java.nio.charset.StandardCharsets;
import java.nio.file.*;
import java.time.*;
import java.util.*;
import java.util.function.*;
Expand Down Expand Up @@ -171,19 +172,6 @@ private void updateWebrevComment(List<Comment> comments, int index, List<WebrevD
}
}

private List<Email> parseArchive(MailingList archive) {
var conversations = archive.conversations(Duration.ofDays(365));

if (conversations.size() == 0) {
return new ArrayList<>();
} else if (conversations.size() == 1) {
var conversation = conversations.get(0);
return conversation.allMessages();
} else {
throw new RuntimeException("Something is wrong with the mbox");
}
}

private EmailAddress getAuthorAddress(CensusInstance censusInstance, HostUser originalAuthor) {
if (bot.ignoredUsers().contains(originalAuthor.username())) {
return bot.emailAddress();
Expand Down Expand Up @@ -255,9 +243,14 @@ public Collection<WorkItem> run(Path scratchPath) {
var path = scratchPath.resolve("mlbridge");
var archiveRepo = materializeArchive(path);
var mboxBasePath = path.resolve(bot.codeRepo().name());
var mbox = MailingListServerFactory.createMboxFileServer(mboxBasePath);
var reviewArchiveList = mbox.getList(pr.id());
var sentMails = parseArchive(reviewArchiveList);

var sentMails = new ArrayList<Email>();
try {
var archiveContents = Files.readString(mboxBasePath.resolve(pr.id() + ".mbox"), StandardCharsets.UTF_8);
sentMails.addAll(Mbox.splitMbox(archiveContents, bot.emailAddress()));
} catch (IOException ignored) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we at least log this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will happen every time a new PR is processed (since there won't be any archive) - I can't think of any actual error that could cause a problem here (problems with the folder contents should have been discovered when cloning it from git)

}

var labels = new HashSet<>(pr.labelNames());

// First determine if this PR should be inspected further or not
Expand Down Expand Up @@ -334,8 +327,6 @@ public Collection<WorkItem> run(Path scratchPath) {

var webrevPath = scratchPath.resolve("mlbridge-webrevs");
var listServer = MailingListServerFactory.createMailmanServer(bot.listArchive(), bot.smtpServer(), bot.sendInterval());
var list = listServer.getList(recipients.get(0).toString());

var archiver = new ReviewArchive(pr, bot.emailAddress());

// Regular comments
Expand Down Expand Up @@ -382,7 +373,13 @@ public Collection<WorkItem> run(Path scratchPath) {
}

// Push all new mails to the archive repository
newMails.forEach(reviewArchiveList::post);
var mbox = MailingListServerFactory.createMboxFileServer(mboxBasePath);
for (var newMail : newMails) {
var forArchiving = Email.from(newMail)
.recipient(EmailAddress.from(pr.id() + "@mbox"))
.build();
mbox.post(forArchiving);
}
pushMbox(archiveRepo, "Adding comments for PR " + bot.codeRepo().name() + "/" + pr.id());

// Finally post all new mails to the actual list
Expand All @@ -396,7 +393,7 @@ public Collection<WorkItem> run(Path scratchPath) {
.headers(bot.headers())
.recipients(recipients)
.build();
list.post(filteredEmail);
listServer.post(filteredEmail);
}
} catch (IOException e) {
throw new UncheckedIOException(e);
Expand Down
14 changes: 6 additions & 8 deletions bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBot.java
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@

public class MailingListArchiveReaderBot implements Bot {
private final EmailAddress archivePoster;
private final Set<MailingList> lists;
private final MailingListReader list;
private final Set<HostedRepository> repositories;
private final Map<EmailAddress, String> parsedConversations = new HashMap<>();
private final Map<EmailAddress, PullRequest> resolvedPullRequests = new HashMap<>();
Expand All @@ -45,9 +45,9 @@ public class MailingListArchiveReaderBot implements Bot {
private final Pattern pullRequestLinkPattern = Pattern.compile("^(?:PR: |Pull request:\\R)(.*?)$", Pattern.MULTILINE);
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.mlbridge");

MailingListArchiveReaderBot(EmailAddress archivePoster, Set<MailingList> lists, Set<HostedRepository> repositories) {
MailingListArchiveReaderBot(EmailAddress archivePoster, MailingListReader list, Set<HostedRepository> repositories) {
this.archivePoster = archivePoster;
this.lists = lists;
this.list = list;
this.repositories = repositories;
}

Expand Down Expand Up @@ -131,11 +131,9 @@ synchronized void inspect(Conversation conversation) {

@Override
public List<WorkItem> getPeriodicItems() {
var readerItems = lists.stream()
.map(list -> new ArchiveReaderWorkItem(this, list))
.collect(Collectors.toList());

var ret = new ArrayList<WorkItem>(readerItems);
var readerItems = new ArchiveReaderWorkItem(this, list);
var ret = new ArrayList<WorkItem>();
ret.add(readerItems);

// Check if there are any potential new comments to post
var item = commentQueue.poll();
Expand Down
28 changes: 14 additions & 14 deletions bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotFactory.java
View file
Open in desktop
Expand Up @@ -88,7 +88,6 @@ public List<Bot> create(BotConfiguration configuration) {
var archiveRef = configuration.repositoryRef(specific.get("archive").asString());
var issueTracker = URIBuilder.base(specific.get("issues").asString()).build();

var listNamesForReading = new HashSet<EmailAddress>();
var allRepositories = new HashSet<HostedRepository>();

var readyLabels = specific.get("ready").get("labels").stream()
Expand All @@ -99,6 +98,7 @@ public List<Bot> create(BotConfiguration configuration) {
.collect(Collectors.toMap(obj -> obj.get("user").asString(),
obj -> Pattern.compile(obj.get("pattern").asString())));
var cooldown = specific.contains("cooldown") ? Duration.parse(specific.get("cooldown").asString()) : Duration.ofMinutes(1);
var mailmanServer = MailingListServerFactory.createMailmanServer(listArchive, listSmtp, Duration.ZERO);

for (var repoConfig : specific.get("repositories").asArray()) {
var repo = repoConfig.get("repository").asString();
Expand All @@ -109,7 +109,20 @@ public List<Bot> create(BotConfiguration configuration) {
repoConfig.get("headers").fields().stream()
.collect(Collectors.toMap(JSONObject.Field::name, field -> field.value().asString())) :
Map.of();

var lists = parseLists(repoConfig.get("lists"));
if (!repoConfig.contains("bidirectional") || repoConfig.get("bidirectional").asBoolean()) {
var listNamesForReading = new HashSet<EmailAddress>();
for (var list : lists) {
listNamesForReading.add(list.list());
}
var listsForReading = listNamesForReading.stream()
.map(EmailAddress::localPart)
.collect(Collectors.toList());
var bot = new MailingListArchiveReaderBot(from, mailmanServer.getListReader(listsForReading.toArray(new String[0])), allRepositories);
ret.add(bot);
}

var folder = repoConfig.contains("folder") ? repoConfig.get("folder").asString() : configuration.repositoryName(repo);

var webrevGenerateHTML = true;
Expand Down Expand Up @@ -156,22 +169,9 @@ public List<Bot> create(BotConfiguration configuration) {
}
ret.add(botBuilder.build());

if (!repoConfig.contains("bidirectional") || repoConfig.get("bidirectional").asBoolean()) {
for (var list : lists) {
listNamesForReading.add(list.list());
}
}
allRepositories.add(configuration.repository(repo));
}

var mailmanServer = MailingListServerFactory.createMailmanServer(listArchive, listSmtp, Duration.ZERO);
var listsForReading = listNamesForReading.stream()
.map(name -> mailmanServer.getList(name.toString()))
.collect(Collectors.toSet());

var bot = new MailingListArchiveReaderBot(from, listsForReading, allRepositories);
ret.add(bot);

return ret;
}
}
28 changes: 14 additions & 14 deletions ...ridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBotTests.java
View file
Open in desktop
Expand Up @@ -38,7 +38,7 @@
import static org.junit.jupiter.api.Assertions.*;

class MailingListArchiveReaderBotTests {
private void addReply(Conversation conversation, EmailAddress recipient, MailingList mailingList, PullRequest pr, String reply) {
private void addReply(Conversation conversation, EmailAddress recipient, MailingListServer mailingListServer, PullRequest pr, String reply) {
var first = conversation.first();
var references = first.id().toString();
var email = Email.create(EmailAddress.from("Commenter", "c@test.test"), "Re: RFR: " + pr.title(), reply)
Expand All @@ -47,11 +47,11 @@ private void addReply(Conversation conversation, EmailAddress recipient, Mailing
.header("In-Reply-To", first.id().toString())
.header("References", references)
.build();
mailingList.post(email);
mailingListServer.post(email);
}

private void addReply(Conversation conversation, EmailAddress recipient, MailingList mailingList, PullRequest pr) {
addReply(conversation, recipient, mailingList, pr, "Looks good");
private void addReply(Conversation conversation, EmailAddress recipient, MailingListServer mailingListServer, PullRequest pr) {
addReply(conversation, recipient, mailingListServer, pr, "Looks good");
}

@Test
Expand Down Expand Up @@ -86,8 +86,8 @@ void simpleArchive(TestInfo testInfo) throws IOException {
// The mailing list as well
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(),
Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var readerBot = new MailingListArchiveReaderBot(from, Set.of(mailmanList), Set.of(archive));
var mailmanList = mailmanServer.getListReader(listAddress.address());
var readerBot = new MailingListArchiveReaderBot(from, mailmanList, Set.of(archive));

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
Expand All @@ -113,7 +113,7 @@ void simpleArchive(TestInfo testInfo) throws IOException {
// Post a reply directly to the list
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
addReply(conversations.get(0), listAddress, mailmanList, pr);
addReply(conversations.get(0), listAddress, mailmanServer, pr);
listServer.processIncoming();

// Another archive reader pass - has to be done twice
Expand Down Expand Up @@ -162,8 +162,8 @@ void rememberBridged(TestInfo testInfo) throws IOException {
// The mailing list as well
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(),
Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var readerBot = new MailingListArchiveReaderBot(from, Set.of(mailmanList), Set.of(archive));
var mailmanList = mailmanServer.getListReader(listAddress.address());
var readerBot = new MailingListArchiveReaderBot(from, mailmanList, Set.of(archive));

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
Expand All @@ -185,7 +185,7 @@ void rememberBridged(TestInfo testInfo) throws IOException {
// Post a reply directly to the list
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
addReply(conversations.get(0), listAddress, mailmanList, pr);
addReply(conversations.get(0), listAddress, mailmanServer, pr);
listServer.processIncoming();

// Another archive reader pass - has to be done twice
Expand All @@ -196,7 +196,7 @@ void rememberBridged(TestInfo testInfo) throws IOException {
var updated = pr.comments();
assertEquals(2, updated.size());

var newReaderBot = new MailingListArchiveReaderBot(from, Set.of(mailmanList), Set.of(archive));
var newReaderBot = new MailingListArchiveReaderBot(from, mailmanList, Set.of(archive));
TestBotRunner.runPeriodicItems(newReaderBot);
TestBotRunner.runPeriodicItems(newReaderBot);

Expand Down Expand Up @@ -238,8 +238,8 @@ void largeEmail(TestInfo testInfo) throws IOException {
// The mailing list as well
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(),
Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var readerBot = new MailingListArchiveReaderBot(from, Set.of(mailmanList), Set.of(archive));
var mailmanList = mailmanServer.getListReader(listAddress.address());
var readerBot = new MailingListArchiveReaderBot(from, mailmanList, Set.of(archive));

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
Expand Down Expand Up @@ -267,7 +267,7 @@ void largeEmail(TestInfo testInfo) throws IOException {
assertEquals(1, conversations.size());

var replyBody = "This line is about 30 bytes long\n".repeat(1000 * 10);
addReply(conversations.get(0), listAddress, mailmanList, pr, replyBody);
addReply(conversations.get(0), listAddress, mailmanServer, pr, replyBody);
listServer.processIncoming();

// Another archive reader pass - has to be done twice
Expand Down