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

843: Mails are not forwarded to a closed PR any more #1103

Closed
wants to merge 6 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
@@ -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;
}
@@ -171,7 +171,7 @@ private void updateWebrevComment(List<Comment> comments, int index, List<WebrevD
}
}

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

if (conversations.size() == 0) {
@@ -256,7 +256,7 @@ public Collection<WorkItem> run(Path scratchPath) {
var archiveRepo = materializeArchive(path);
var mboxBasePath = path.resolve(bot.codeRepo().name());
var mbox = MailingListServerFactory.createMboxFileServer(mboxBasePath);
var reviewArchiveList = mbox.getList(pr.id());
var reviewArchiveList = mbox.getListReader(pr.id());
var sentMails = parseArchive(reviewArchiveList);
var labels = new HashSet<>(pr.labelNames());

@@ -334,8 +334,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
@@ -382,7 +380,12 @@ public Collection<WorkItem> run(Path scratchPath) {
}

// Push all new mails to the archive repository
newMails.forEach(reviewArchiveList::post);
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
@@ -396,7 +399,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);
@@ -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<>();
@@ -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;
}

@@ -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();
@@ -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()
@@ -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();
@@ -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;
@@ -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;
}
}
@@ -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)
@@ -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
@@ -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());
@@ -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
@@ -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());
@@ -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
@@ -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);

@@ -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());
@@ -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
@@ -214,7 +214,7 @@ void simpleArchive(TestInfo testInfo) throws IOException {
// The mailing list as well
listServer.processIncoming();
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();
@@ -796,7 +796,7 @@ void reviewComment(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 mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();
@@ -884,7 +884,7 @@ void combineComments(TestInfo testInfo) throws IOException {

// As well as the mailing list
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();
@@ -1001,7 +1001,7 @@ void commentThreading(TestInfo testInfo) throws IOException {

// Check the mailing list
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();
@@ -1607,7 +1607,7 @@ void incrementalChanges(TestInfo testInfo) throws IOException {

// Check that sender address is set properly
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
for (var newMail : conversations.get(0).allMessages()) {
@@ -1734,7 +1734,7 @@ void rebased(TestInfo testInfo) throws IOException {

// Check that sender address is set properly
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
for (var newMail : conversations.get(0).allMessages()) {
@@ -2899,7 +2899,7 @@ void multipleRecipients(TestInfo testInfo) throws IOException {

// The mail should have been sent to list1
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress1.address());
var mailmanList = mailmanServer.getListReader(listAddress1.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();
@@ -3031,7 +3031,7 @@ void jsonArchive(TestInfo testInfo) throws IOException {
// The mailing list as well
listServer.processIncoming();
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getList(listAddress.address());
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();