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

321: No RFR mail is sent if a PR is integrated very quickly #538

Closed
wants to merge 2 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
@@ -49,9 +49,11 @@ private static Optional<Commit> 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)) {
@@ -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;
}
}
@@ -197,8 +197,8 @@ public List<WorkItem> 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();
}

@@ -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<ArchiveItem> generateArchiveItems(List<Email> 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<ArchiveItem> generateArchiveItems(List<Email> 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<ArchiveItem> generateArchiveItems(List<Email> 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);
}
}
@@ -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());
}
}
}
}
@@ -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);
@@ -161,7 +161,6 @@ List<TestPullRequest> 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());
}

@@ -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<PullRequest> pullRequests() {
return new ArrayList<>(host.getPullRequests(this));
return host.getPullRequests(this).stream()
.filter(pr -> pr.state().equals(Issue.State.OPEN))
.collect(Collectors.toList());
}

@Override