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

993: Stuck merge PR (Panama) #1138

Closed
wants to merge 1 commit 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
@@ -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;