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

375: Improve Merge PR RFR email #591

Closed
wants to merge 8 commits into from
Closed
Changes from 2 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
@@ -38,30 +38,32 @@ private ArchiveItem(ArchiveItem parent, String id, ZonedDateTime created, ZonedD
this.footer = footer;
}

private static Optional<Commit> mergeCommit(PullRequestInstance prInstance, Hash head) {
private static Optional<Commit> mergeCommit(PullRequest pr, Repository localRepo, Hash head) {
try {
var author = new Author("duke", "duke@openjdk.org");
var hash = prInstance.commit(head, author, author, prInstance.pr().title());
return prInstance.localRepo().lookup(hash);
var prUtils = new PullRequestUtils(pr);
var hash = prUtils.createCommit(localRepo, head, author, author, pr.title());
return localRepo.lookup(hash);
} catch (IOException | CommitFailure e) {
return Optional.empty();
}
}

static ArchiveItem from(PullRequestInstance prInstance, HostUserToEmailAuthor hostUserToEmailAuthor,
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, String threadPrefix) {
return new ArchiveItem(null, "fc", created, updated, prInstance.pr().author(), Map.of("PR-Head-Hash", head.hex(),
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 + (threadPrefix.isEmpty() ? "" : ": ") + prInstance.pr().title(),
() -> subjectPrefix + threadPrefix + (threadPrefix.isEmpty() ? "" : ": ") + pr.title(),
() -> "",
() -> ArchiveMessages.composeConversation(prInstance.pr()),
() -> ArchiveMessages.composeConversation(pr),
() -> {
if (prInstance.isMerge()) {
var prUtils = new PullRequestUtils(pr);
if (prUtils.isMerge()) {
//TODO: Try to merge in target - generate possible conflict webrev
var mergeCommit = mergeCommit(prInstance, head);
var mergeCommit = mergeCommit(pr, localRepo, head);
var mergeWebrevs = new ArrayList<WebrevDescription>();
if (mergeCommit.isPresent()) {
for (int i = 0; i < mergeCommit.get().parentDiffs().size(); ++i) {
@@ -71,10 +73,10 @@ static ArchiveItem from(PullRequestInstance prInstance, HostUserToEmailAuthor ho
}
switch (i) {
case 0:
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_TARGET, prInstance.pr().targetRef()));
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_TARGET, pr.targetRef()));
break;
case 1:
var mergeSource = prInstance.pr().title().length() > 6 ? prInstance.pr().title().substring(6) : null;
var mergeSource = pr.title().length() > 6 ? pr.title().substring(6) : null;
mergeWebrevs.add(webrevGenerator.generate(diff, String.format("00.%d", i), WebrevDescription.Type.MERGE_SOURCE, mergeSource));
break;
default:
@@ -86,11 +88,11 @@ static ArchiveItem from(PullRequestInstance prInstance, HostUserToEmailAuthor ho
webrevNotification.notify(0, mergeWebrevs);
}
}
return ArchiveMessages.composeMergeConversationFooter(prInstance.pr(), prInstance.localRepo(), mergeWebrevs, base, head);
return ArchiveMessages.composeMergeConversationFooter(pr, localRepo, mergeWebrevs, base, head);
} else {
var fullWebrev = webrevGenerator.generate(base, head, "00", WebrevDescription.Type.FULL);
webrevNotification.notify(0, List.of(fullWebrev));
return ArchiveMessages.composeConversationFooter(prInstance.pr(), issueTracker, issuePrefix, prInstance.localRepo(), fullWebrev, base, head);
return ArchiveMessages.composeConversationFooter(pr, issueTracker, issuePrefix, localRepo, fullWebrev, base, head);
}
});
}
@@ -293,13 +293,14 @@ public void run(Path scratchPath) {
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepoPath = scratchPath.resolve("mlbridge-mergebase");
var prInstance = new PullRequestInstance(localRepoPath, hostedRepositoryPool, pr);
var localRepo = hostedRepositoryPool.checkout(pr, localRepoPath.resolve(pr.repository().name()));
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":archiveworkitem", false);

var webrevPath = scratchPath.resolve("mlbridge-webrevs");
var listServer = MailingListServerFactory.createMailmanServer(bot.listArchive(), bot.smtpServer(), bot.sendInterval());
var list = listServer.getList(bot.listAddress().address());

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

// Regular comments
for (var comment : comments) {
@@ -327,8 +328,8 @@ public void run(Path scratchPath) {
archiver.addReviewComment(reviewComment);
}

var webrevGenerator = bot.webrevStorage().generator(pr, prInstance.localRepo(), webrevPath);
var newMails = archiver.generateNewEmails(sentMails, bot.cooldown(), prInstance.localRepo(), bot.issueTracker(), jbs.toUpperCase(), webrevGenerator,
var webrevGenerator = bot.webrevStorage().generator(pr, localRepo, webrevPath);
var newMails = archiver.generateNewEmails(sentMails, bot.cooldown(), localRepo, bot.issueTracker(), jbs.toUpperCase(), webrevGenerator,
(index, webrevs) -> updateWebrevComment(comments, index, webrevs),
user -> getAuthorAddress(census, user),
user -> getAuthorUserName(census, user),
@@ -6,6 +6,7 @@
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.vcs.*;

import java.io.IOException;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.security.*;
@@ -16,7 +17,7 @@
import java.util.stream.*;

class ReviewArchive {
private final PullRequestInstance prInstance;
private final PullRequest pr;
private final EmailAddress sender;

private final List<Comment> comments = new ArrayList<>();
@@ -25,8 +26,8 @@ class ReviewArchive {

private final Logger log = Logger.getLogger("org.openjdk.skara.bots.mlbridge");

ReviewArchive(PullRequestInstance prInstance, EmailAddress sender) {
this.prInstance = prInstance;
ReviewArchive(PullRequest pr, EmailAddress sender) {
this.pr = pr;
this.sender = sender;
}

@@ -51,7 +52,7 @@ private Optional<ArchiveItem> findPreviousReplyBy(List<ArchiveItem> generated, H
.findAny();
}

private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repository localRepo, URI issueTracker, String issuePrefix, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, String subjectPrefix) {
private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repository localRepo, URI issueTracker, String issuePrefix, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, String subjectPrefix) throws IOException {
var generated = new ArrayList<ArchiveItem>();
Hash lastBase = null;
Hash lastHead = null;
@@ -64,9 +65,9 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
threadPrefix = first.headerValue("PR-Thread-Prefix");
}
} else {
if (prInstance.pr().state() != Issue.State.OPEN) {
if (pr.state() != Issue.State.OPEN) {
threadPrefix = "FYI";
} else if (prInstance.pr().labels().contains("failed-auto-merge")) {
} else if (pr.labels().contains("failed-auto-merge")) {
threadPrefix = "";
}
}
@@ -79,10 +80,10 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
var created = email.date();

if (generated.isEmpty()) {
var first = ArchiveItem.from(prInstance, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, prInstance.pr().createdAt(), prInstance.pr().updatedAt(), curBase, curHead, subjectPrefix, threadPrefix);
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(prInstance.pr(), localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, created, created, lastBase, lastHead, curBase, curHead, ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
generated.add(revision);
}

@@ -92,39 +93,41 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
}

// Check if we're at a revision not previously reported
if (!prInstance.baseHash().equals(lastBase) || !prInstance.headHash().equals(lastHead)) {
var prUtils = new PullRequestUtils(pr);
var baseHash = prUtils.baseHash(localRepo);
if (!baseHash.equals(lastBase) || !pr.headHash().equals(lastHead)) {
if (generated.isEmpty()) {
var first = ArchiveItem.from(prInstance, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, prInstance.pr().createdAt(), prInstance.pr().updatedAt(), prInstance.baseHash(), prInstance.headHash(), subjectPrefix, threadPrefix);
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), baseHash, pr.headHash(), subjectPrefix, threadPrefix);
generated.add(first);
} else {
var revision = ArchiveItem.from(prInstance.pr(), localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, prInstance.pr().updatedAt(), prInstance.pr().updatedAt(), lastBase, lastHead, prInstance.baseHash(), prInstance.headHash(), ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
var revision = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, webrevGenerator, webrevNotification, pr.updatedAt(), pr.updatedAt(), lastBase, lastHead, baseHash, pr.headHash(), ++revisionIndex, generated.get(0), subjectPrefix, threadPrefix);
generated.add(revision);
}
}

// A review always have a revision mail as parent, so start with these
for (var review : reviews) {
var parent = ArchiveItem.findParent(generated, review);
var reply = ArchiveItem.from(prInstance.pr(), review, hostUserToEmailAuthor, hostUserToUserName, hostUserToRole, parent);
var reply = ArchiveItem.from(pr, review, hostUserToEmailAuthor, hostUserToUserName, hostUserToRole, parent);
generated.add(reply);
}
// Comments have either a comment or a review as parent, the eligible ones have been generated at this point
for (var comment : comments) {
var parent = ArchiveItem.findParent(generated, comment);
var reply = ArchiveItem.from(prInstance.pr(), comment, hostUserToEmailAuthor, parent);
var reply = ArchiveItem.from(pr, comment, hostUserToEmailAuthor, parent);
generated.add(reply);
}
// Finally, file specific comments should be seen after general review comments
for (var reviewComment : reviewComments) {
var parent = ArchiveItem.findParent(generated, reviewComments, reviewComment);
var reply = ArchiveItem.from(prInstance.pr(), reviewComment, hostUserToEmailAuthor, parent);
var reply = ArchiveItem.from(pr, reviewComment, hostUserToEmailAuthor, parent);
generated.add(reply);
}

// Post a closed notice for regular RFR threads that weren't integrated
if ((prInstance.pr().state() != Issue.State.OPEN) && threadPrefix.equals("RFR") && !prInstance.pr().labels().contains("integrated")) {
if ((pr.state() != Issue.State.OPEN) && threadPrefix.equals("RFR") && !pr.labels().contains("integrated")) {
var parent = generated.get(0);
var reply = ArchiveItem.closedNotice(prInstance.pr(), hostUserToEmailAuthor, parent, subjectPrefix, threadPrefix);
var reply = ArchiveItem.closedNotice(pr, hostUserToEmailAuthor, parent, subjectPrefix, threadPrefix);
generated.add(reply);
}

@@ -210,13 +213,13 @@ private Email findArchiveItemEmail(ArchiveItem item, List<Email> sentEmails, Lis

private EmailAddress getUniqueMessageId(String identifier) {
try {
var prSpecific = prInstance.pr().repository().name().replace("/", ".") + "." + prInstance.pr().id();
var prSpecific = pr.repository().name().replace("/", ".") + "." + pr.id();
var digest = MessageDigest.getInstance("SHA-256");
digest.update(prSpecific.getBytes(StandardCharsets.UTF_8));
digest.update(identifier.getBytes(StandardCharsets.UTF_8));
var encodedCommon = Base64.getUrlEncoder().encodeToString(digest.digest());

return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + prInstance.pr().repository().url().getHost());
return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().url().getHost());
} catch (NoSuchAlgorithmException e) {
throw new RuntimeException("Cannot find SHA-256");
}
@@ -226,7 +229,7 @@ private String getStableMessageId(EmailAddress uniqueMessageId) {
return uniqueMessageId.localPart().split("\\.")[0];
}

List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, String subjectPrefix, Consumer<Instant> retryConsumer) {
List<Email> generateNewEmails(List<Email> sentEmails, Duration cooldown, Repository localRepo, URI issueTracker, String issuePrefix, WebrevStorage.WebrevGenerator webrevGenerator, WebrevNotification webrevNotification, HostUserToEmailAuthor hostUserToEmailAuthor, HostUserToUserName hostUserToUserName, HostUserToRole hostUserToRole, String subjectPrefix, Consumer<Instant> retryConsumer) throws IOException {
var ret = new ArrayList<Email>();
var allItems = generateArchiveItems(sentEmails, localRepo, issueTracker, issuePrefix, hostUserToEmailAuthor, hostUserToUserName, hostUserToRole, webrevGenerator, webrevNotification, subjectPrefix);
var sentItemIds = sentItemIds(sentEmails);