Skip to content
Permalink
Browse files
993: Stuck merge PR (Panama)
Reviewed-by: rwestberg
  • Loading branch information
edvbld committed Apr 27, 2021
1 parent f084dc2 commit 4d271a08a3896d6fa6e6791227c5165c58c2e0c2
Showing 3 changed files with 4 additions and 134 deletions.
@@ -43,7 +43,6 @@
import java.util.logging.Logger;

class MergeBot implements Bot, WorkItem {
private final String integrationCommand = "/integrate\n<!-- Valid self-command -->";
private final Logger log = Logger.getLogger("org.openjdk.skara.bots");;
private final Path storage;

@@ -321,42 +320,6 @@ public Collection<WorkItem> run(Path scratchPath) {
// Yes, this could be optimized do a merge "this turn", but it is much simpler
// to just wait until the next time the bot runs
shouldMerge = false;

if (pr.labelNames().contains("ready") && !pr.labelNames().contains("sponsor")) {
var comments = pr.comments();
var integrateComments =
comments.stream()
.filter(c -> c.author().equals(currentUser))
.filter(c -> c.body().equals(integrationCommand))
.collect(Collectors.toList());
if (integrateComments.isEmpty()) {
pr.addComment(integrationCommand);
} else {
var lastIntegrateComment = integrateComments.get(integrateComments.size() - 1);
var id = lastIntegrateComment.id();
var botUserId = "43336822";
var replyMarker = "<!-- Jmerge command reply message (" + id + ") -->";
var replies = comments.stream()
.filter(c -> c.author().id().equals(botUserId))
.filter(c -> c.body().startsWith(replyMarker))
.collect(Collectors.toList());
if (replies.isEmpty()) {
// No reply yet, just wait
} else {
// Got a reply and the "sponsor" label is not present, check for error
// and if we should add the `/integrate` command again
var lastReply = replies.get(replies.size() - 1);
var lines = lastReply.body().split("\n");
var errorPrefix = "@openjdk-bot Your integration request cannot be fulfilled at this time";
if (lines.length > 1 && lines[1].startsWith(errorPrefix)) {
// Try again
pr.addComment(integrationCommand);
}
// Other reply, potentially due to rebase issue, just
// wait for the labeler to add appropriate labels.
}
}
}
}
}

@@ -617,6 +580,8 @@ public Collection<WorkItem> run(Path scratchPath) {
message.add("");
message.add("Thanks,");
message.add("J. Duke");
message.add("");
message.add("/integrate auto");

var prFromFork = fork.createPullRequest(prTarget,
toBranch.name(),
@@ -569,101 +569,6 @@ void failingMergeShouldResultInOnlyOnePR(TestInfo testInfo) throws IOException {
}
}

@Test
void resolvedMergeConflictShouldResultInIntegrateCommand(TestInfo testInfo) throws IOException {
try (var temp = new TemporaryDirectory()) {
var host = TestHost.createNew(List.of(HostUser.create(0, "duke", "J. Duke")));

var fromDir = temp.path().resolve("from.git");
var fromLocalRepo = TestableRepository.init(fromDir, VCS.GIT);
var fromHostedRepo = new TestHostedRepository(host, "test", fromLocalRepo);

var toDir = temp.path().resolve("to.git");
var toLocalRepo = TestableRepository.init(toDir, VCS.GIT);
var toGitConfig = toDir.resolve(".git").resolve("config");
Files.write(toGitConfig, List.of("[receive]", "denyCurrentBranch = ignore"),
StandardOpenOption.APPEND);
var toHostedRepo = new TestHostedRepository(host, "test-mirror", toLocalRepo);

var forkDir = temp.path().resolve("fork.git");
var forkLocalRepo = TestableRepository.init(forkDir, VCS.GIT);
var forkGitConfig = forkDir.resolve(".git").resolve("config");
Files.write(forkGitConfig, List.of("[receive]", "denyCurrentBranch = ignore"),
StandardOpenOption.APPEND);
var toFork = new TestHostedRepository(host, "test-mirror-fork", forkLocalRepo);

var now = ZonedDateTime.now();
var fromFileA = fromDir.resolve("a.txt");
Files.writeString(fromFileA, "Hello A\n");
fromLocalRepo.add(fromFileA);
var fromHashA = fromLocalRepo.commit("Adding a.txt", "duke", "duke@openjdk.org", now);
var fromCommits = fromLocalRepo.commits().asList();
assertEquals(1, fromCommits.size());
assertEquals(fromHashA, fromCommits.get(0).hash());

var toFileA = toDir.resolve("a.txt");
Files.writeString(toFileA, "Hello A\n");
toLocalRepo.add(toFileA);
var toHashA = toLocalRepo.commit("Adding a.txt", "duke", "duke@openjdk.org", now);
var toCommits = toLocalRepo.commits().asList();
assertEquals(1, toCommits.size());
assertEquals(toHashA, toCommits.get(0).hash());
assertEquals(fromHashA, toHashA);

var fromFileB = fromDir.resolve("b.txt");
Files.writeString(fromFileB, "Hello B1\n");
fromLocalRepo.add(fromFileB);
var fromHashB = fromLocalRepo.commit("Adding b1.txt", "duke", "duke@openjdk.org", now);

var toFileB = toDir.resolve("b.txt");
Files.writeString(toFileB, "Hello B2\n");
toLocalRepo.add(toFileB);
var toHashB = toLocalRepo.commit("Adding b2.txt", "duke", "duke@openjdk.org", now);

var storage = temp.path().resolve("storage");
var master = new Branch("master");
var specs = List.of(new MergeBot.Spec(fromHostedRepo, master, master));
var bot = new MergeBot(storage, toHostedRepo, toFork, specs);
TestBotRunner.runPeriodicItems(bot);
TestBotRunner.runPeriodicItems(bot);

toCommits = toLocalRepo.commits().asList();
assertEquals(2, toCommits.size());
var toHashes = toCommits.stream().map(Commit::hash).collect(Collectors.toSet());
assertTrue(toHashes.contains(toHashA));
assertTrue(toHashes.contains(toHashB));

var pullRequests = toHostedRepo.pullRequests();
assertEquals(1, pullRequests.size());
var pr = pullRequests.get(0);
assertEquals("Merge test:master", pr.title());
assertTrue(pr.labelNames().contains("failed-auto-merge"));
assertTrue(forkLocalRepo.branches().contains(new Branch("master")));
assertTrue(forkLocalRepo.branches().contains(new Branch("2")));

// Bot should do nothing as long as PR is presnt
TestBotRunner.runPeriodicItems(bot);
pullRequests = toHostedRepo.pullRequests();
assertEquals(1, pullRequests.size());
pr = pullRequests.get(0);

// Simulate that the merge-conflict has been resolved by adding the "ready" label
pr.addLabel("ready");
TestBotRunner.runPeriodicItems(bot);
pullRequests = toHostedRepo.pullRequests();
assertEquals(1, pullRequests.size());

pr = pullRequests.get(0);
var numComments = pr.comments().size();
var lastComment = pr.comments().get(pr.comments().size() - 1);
assertEquals("/integrate\n<!-- Valid self-command -->", lastComment.body());

// Running the bot again should not result in another comment
TestBotRunner.runPeriodicItems(bot);
assertEquals(numComments, toHostedRepo.pullRequests().size());
}
}

final static class TestClock implements Clock {
ZonedDateTime now;

@@ -53,13 +53,13 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments);
if (readyHash.isEmpty()) {
if (!pr.labelNames().contains("auto") && readyHash.isEmpty()) {
reply.println("The change author (@" + pr.author().username() + ") must issue an `integrate` command before the integration can be sponsored.");
return;
}

var acceptedHash = readyHash.get();
if (!pr.headHash().equals(acceptedHash)) {
if (!pr.labelNames().contains("auto") && !pr.headHash().equals(acceptedHash)) {
reply.print("The PR has been updated since the change author (@" + pr.author().username() + ") ");
reply.println("issued the `integrate` command - the author must perform this command again.");
return;

1 comment on commit 4d271a0

@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented on 4d271a0 Apr 27, 2021

Choose a reason for hiding this comment

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

Please sign in to comment.