diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java index f26d51141..b06b75afe 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveItem.java @@ -49,9 +49,11 @@ private static Optional mergeCommit(Repository localRepo, Hash head) { static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAuthor hostUserToEmailAuthor, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, ZonedDateTime created, ZonedDateTime updated, - Hash base, Hash head, String subjectPrefix) { - return new ArchiveItem(null, "fc", created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()), - () -> subjectPrefix + "RFR: " + pr.title(), + Hash base, Hash head, String subjectPrefix, String threadPrefix) { + return new ArchiveItem(null, "fc", created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(), + "PR-Base-Hash", base.hex(), + "PR-Thread-Prefix", threadPrefix), + () -> subjectPrefix + threadPrefix + ": " + pr.title(), () -> "", () -> ArchiveMessages.composeConversation(pr, localRepo, base, head), () -> { @@ -108,9 +110,9 @@ private static String hostUserToCommitterName(HostUserToEmailAuthor hostUserToEm static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAuthor hostUserToEmailAuthor, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, ZonedDateTime created, ZonedDateTime updated, Hash lastBase, Hash lastHead, Hash base, - Hash head, int index, ArchiveItem parent, String subjectPrefix) { + Hash head, int index, ArchiveItem parent, String subjectPrefix, String threadPrefix) { return new ArchiveItem(parent,"ha" + head.hex(), created, updated, pr.author(), Map.of("PR-Head-Hash", head.hex(), "PR-Base-Hash", base.hex()), - () -> String.format("Re: %s[Rev %02d] RFR: %s", subjectPrefix, index, pr.title()), + () -> String.format("Re: %s[Rev %02d] %s: %s", subjectPrefix, index, threadPrefix, pr.title()), () -> "", () -> { if (lastBase.equals(base)) { diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java index 988adc9d6..8f611a658 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java @@ -26,7 +26,7 @@ import org.openjdk.skara.email.*; import org.openjdk.skara.forge.*; import org.openjdk.skara.host.HostUser; -import org.openjdk.skara.issuetracker.Comment; +import org.openjdk.skara.issuetracker.*; import org.openjdk.skara.mailinglist.*; import org.openjdk.skara.vcs.Repository; @@ -243,9 +243,17 @@ public void run(Path scratchPath) { // First determine if this PR should be inspected further or not if (sentMails.isEmpty()) { var labels = new HashSet<>(pr.labels()); - for (var readyLabel : bot.readyLabels()) { - if (!labels.contains(readyLabel)) { - log.fine("PR is not yet ready - missing label '" + readyLabel + "'"); + + if (pr.state() == Issue.State.OPEN) { + for (var readyLabel : bot.readyLabels()) { + if (!labels.contains(readyLabel)) { + log.fine("PR is not yet ready - missing label '" + readyLabel + "'"); + return; + } + } + } else { + if (!labels.contains("integrated")) { + log.fine("Closed PR was not integrated - will not initiate an RFR thread"); return; } } diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBot.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBot.java index c7519cf07..a91fb3e6e 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBot.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBot.java @@ -197,8 +197,8 @@ public List getPeriodicItems() { log.info("Fetching all open pull requests"); prs = codeRepo.pullRequests(); } else { - log.info("Fetching only pull requests updated after " + lastPartialUpdate.minus(cooldown)); - prs = codeRepo.pullRequests(lastPartialUpdate.minus(cooldown)); + log.info("Fetching recently updated pull requests (open and closed)"); + prs = codeRepo.pullRequests(ZonedDateTime.now().minus(Duration.ofDays(14))); lastPartialUpdate = ZonedDateTime.now(); } diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java index 5c1ebb02e..702e30c80 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ReviewArchive.java @@ -3,7 +3,7 @@ import org.openjdk.skara.email.*; import org.openjdk.skara.forge.*; import org.openjdk.skara.host.HostUser; -import org.openjdk.skara.issuetracker.Comment; +import org.openjdk.skara.issuetracker.*; import org.openjdk.skara.vcs.*; import java.net.URI; @@ -60,6 +60,18 @@ private List generateArchiveItems(List sentEmails, Repositor Hash lastBase = null; Hash lastHead = null; int revisionIndex = 0; + var threadPrefix = "RFR"; + + if (!sentEmails.isEmpty()) { + var first = sentEmails.get(0); + if (first.hasHeader("PR-Thread-Prefix")) { + threadPrefix = first.headerValue("PR-Thread-Prefix"); + } + } else { + if (pr.state() != Issue.State.OPEN) { + threadPrefix = "FYI"; + } + } // Check existing generated mails to find which hashes have been previously reported for (var email : sentEmails) { @@ -69,10 +81,10 @@ private List generateArchiveItems(List sentEmails, Repositor var created = email.date(); if (generated.isEmpty()) { - var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), curBase, curHead, subjectPrefix); + var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), curBase, curHead, subjectPrefix, threadPrefix); generated.add(first); } else { - var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0), subjectPrefix); + var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix); generated.add(revision); } @@ -84,10 +96,10 @@ private List generateArchiveItems(List sentEmails, Repositor // Check if we're at a revision not previously reported if (!base.equals(lastBase) || !head.equals(lastHead)) { if (generated.isEmpty()) { - var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), base, head, subjectPrefix); + var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), base, head, subjectPrefix, threadPrefix); generated.add(first); } else { - var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, base, head, ++revisionIndex, generated.get(0), subjectPrefix); + var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, base, head, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix); generated.add(revision); } } diff --git a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java index bb5299c6c..fa636248f 100644 --- a/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java +++ b/bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/WebrevStorage.java @@ -78,10 +78,17 @@ private void generate(PullRequest pr, Repository localRepository, Path folder, D var conf = JCheckConfiguration.from(localRepository, head); var project = conf.general().jbs() != null ? conf.general().jbs() : conf.general().project(); var id = issue.get().id(); - var issueTracker = IssueTracker.from("jira", URI.create("https://bugs.openjdk.java.net")); - var hostedIssue = issueTracker.project(project).issue(id); - if (hostedIssue.isPresent()) { - builder = builder.issue(hostedIssue.get().webUrl().toString()); + IssueTracker issueTracker = null; + try { + issueTracker = IssueTracker.from("jira", URI.create("https://bugs.openjdk.java.net")); + } catch (RuntimeException e) { + log.warning("Failed to create Jira issue tracker"); + } + if (issueTracker != null) { + var hostedIssue = issueTracker.project(project).issue(id); + if (hostedIssue.isPresent()) { + builder = builder.issue(hostedIssue.get().webUrl().toString()); + } } } } diff --git a/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java b/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java index d7378e0f6..1d83a2b7e 100644 --- a/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java +++ b/bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java @@ -24,6 +24,7 @@ import org.openjdk.skara.email.EmailAddress; import org.openjdk.skara.forge.*; +import org.openjdk.skara.issuetracker.Issue; import org.openjdk.skara.network.URIBuilder; import org.openjdk.skara.mailinglist.MailingListServerFactory; import org.openjdk.skara.test.*; @@ -269,6 +270,159 @@ void simpleArchive(TestInfo testInfo) throws IOException { } } + @Test + void archiveIntegrated(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory(); + var archiveFolder = new TemporaryDirectory(); + var webrevFolder = new TemporaryDirectory(); + var listServer = new TestMailmanServer(); + var webrevServer = new TestWebrevServer()) { + var author = credentials.getHostedRepository(); + var archive = credentials.getHostedRepository(); + var ignored = credentials.getHostedRepository(); + var listAddress = EmailAddress.parse(listServer.createList("test")); + var censusBuilder = credentials.getCensusBuilder() + .addAuthor(author.forge().currentUser().id()); + var from = EmailAddress.from("test", "test@test.mail"); + var mlBot = MailingListBridgeBot.newBuilder() + .from(from) + .repo(author) + .archive(archive) + .censusRepo(censusBuilder.build()) + .list(listAddress) + .ignoredUsers(Set.of(ignored.forge().currentUser().userName())) + .listArchive(listServer.getArchive()) + .smtpServer(listServer.getSMTP()) + .webrevStorageRepository(archive) + .webrevStorageRef("webrev") + .webrevStorageBase(Path.of("test")) + .webrevStorageBaseUri(webrevServer.uri()) + .issueTracker(URIBuilder.base("http://issues.test/browse/").build()) + .build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + localRepo.push(masterHash, author.url(), "master", true); + localRepo.push(masterHash, archive.url(), "webrev", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo, "A simple change", + "Change msg\n\nWith several lines"); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(archive, "master", "edit", "1234: This is a pull request"); + pr.setBody("This should not be ready"); + pr.setState(Issue.State.CLOSED); + + // Run an archive pass + TestBotRunner.runPeriodicItems(mlBot); + TestBotRunner.runPeriodicItems(mlBot); + + // A PR that isn't ready for review should not be archived + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertFalse(archiveContains(archiveFolder.path(), "This is a pull request")); + + // Flag it as ready for review + pr.setBody("This has already been integrated"); + pr.addLabel("integrated"); + + // Run another archive pass + TestBotRunner.runPeriodicItems(mlBot); + + // The archive should now contain an entry + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertTrue(archiveContains(archiveFolder.path(), "Subject: FYI: 1234: This is a pull request")); + + var updatedHash = CheckableRepository.appendAndCommit(localRepo, "Another change"); + localRepo.push(updatedHash, author.url(), "edit"); + + // Run another archive pass + TestBotRunner.runPeriodicItems(mlBot); + + // The archive should now contain another entry + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertTrue(archiveContains(archiveFolder.path(), "Subject: Re: \\[Rev 01\\] FYI: 1234: This is a pull request")); + } + } + + @Test + void archiveIntegratedRetainPrefix(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory(); + var archiveFolder = new TemporaryDirectory(); + var webrevFolder = new TemporaryDirectory(); + var listServer = new TestMailmanServer(); + var webrevServer = new TestWebrevServer()) { + var author = credentials.getHostedRepository(); + var archive = credentials.getHostedRepository(); + var ignored = credentials.getHostedRepository(); + var listAddress = EmailAddress.parse(listServer.createList("test")); + var censusBuilder = credentials.getCensusBuilder() + .addAuthor(author.forge().currentUser().id()); + var from = EmailAddress.from("test", "test@test.mail"); + var mlBot = MailingListBridgeBot.newBuilder() + .from(from) + .repo(author) + .archive(archive) + .censusRepo(censusBuilder.build()) + .list(listAddress) + .ignoredUsers(Set.of(ignored.forge().currentUser().userName())) + .listArchive(listServer.getArchive()) + .smtpServer(listServer.getSMTP()) + .webrevStorageRepository(archive) + .webrevStorageRef("webrev") + .webrevStorageBase(Path.of("test")) + .webrevStorageBaseUri(webrevServer.uri()) + .readyLabels(Set.of("rfr")) + .issueTracker(URIBuilder.base("http://issues.test/browse/").build()) + .build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + localRepo.push(masterHash, author.url(), "master", true); + localRepo.push(masterHash, archive.url(), "webrev", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo, "A simple change", + "Change msg\n\nWith several lines"); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(archive, "master", "edit", "1234: This is a pull request"); + pr.setBody("This should be ready"); + + // Run an archive pass + TestBotRunner.runPeriodicItems(mlBot); + TestBotRunner.runPeriodicItems(mlBot); + + // A PR that isn't ready for review should not be archived + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertFalse(archiveContains(archiveFolder.path(), "This is a pull request")); + + // Flag it as ready for review + pr.addLabel("rfr"); + + // Run another archive pass + TestBotRunner.runPeriodicItems(mlBot); + + // The archive should now contain an entry + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertTrue(archiveContains(archiveFolder.path(), "Subject: RFR: 1234: This is a pull request")); + + // Close it and then push another change + pr.setState(Issue.State.CLOSED); + var updatedHash = CheckableRepository.appendAndCommit(localRepo, "Another change"); + localRepo.push(updatedHash, author.url(), "edit"); + + // Run another archive pass + TestBotRunner.runPeriodicItems(mlBot); + + // The archive should now contain another entry - should retain the RFR thread prefix + Repository.materialize(archiveFolder.path(), archive.url(), "master"); + assertTrue(archiveContains(archiveFolder.path(), "Subject: Re: \\[Rev 01\\] RFR: 1234: This is a pull request")); + } + } + @Test void reviewComment(TestInfo testInfo) throws IOException { try (var credentials = new HostCredentials(testInfo); diff --git a/test/src/main/java/org/openjdk/skara/test/TestHost.java b/test/src/main/java/org/openjdk/skara/test/TestHost.java index b4cd6eaac..8650b586a 100644 --- a/test/src/main/java/org/openjdk/skara/test/TestHost.java +++ b/test/src/main/java/org/openjdk/skara/test/TestHost.java @@ -161,7 +161,6 @@ List getPullRequests(TestHostedRepository repository) { return data.pullRequests.entrySet().stream() .sorted(Comparator.comparing(Map.Entry::getKey)) .map(pr -> getPullRequest(repository, pr.getKey())) - .filter(pr -> pr.state().equals(Issue.State.OPEN)) .collect(Collectors.toList()); } diff --git a/test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java b/test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java index 763eada92..4ab7d04d3 100644 --- a/test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java +++ b/test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java @@ -23,6 +23,7 @@ package org.openjdk.skara.test; import org.openjdk.skara.forge.*; +import org.openjdk.skara.issuetracker.Issue; import org.openjdk.skara.json.JSONValue; import org.openjdk.skara.vcs.*; @@ -70,7 +71,9 @@ public PullRequest pullRequest(String id) { @Override public List pullRequests() { - return new ArrayList<>(host.getPullRequests(this)); + return host.getPullRequests(this).stream() + .filter(pr -> pr.state().equals(Issue.State.OPEN)) + .collect(Collectors.toList()); } @Override