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.

Sign up for GitHub

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

Jump to bottom

1069: PR ends up in bad state if interrupted just after push #1183

Closed
wants to merge 3 commits into from
Closed

1069: PR ends up in bad state if interrupted just after push #1183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
746585d
SKARA-1069
erikj79 Jun 7, 2021
abd81e3
Merge branch 'master' into SKARA-1069-push-interrupt
erikj79 Jun 9, 2021
fd4a883
Review comments and fix SponsorCommand
erikj79 Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 78 additions & 19 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.openjdk.skara.forge.*;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.vcs.Hash;
import org.openjdk.skara.vcs.Repository;

import java.io.*;
import java.nio.file.Path;
Expand All @@ -38,8 +39,10 @@
import java.util.regex.Matcher;

public class IntegrateCommand implements CommandHandler {
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
private final static Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
private static final Pattern BACKPORT_PATTERN = Pattern.compile("<!-- backport ([0-9a-z]{40}) -->");
private static final String PRE_PUSH_MARKER = "<!-- prepush %s -->";
private static final Pattern PRE_PUSH_PATTERN = Pattern.compile("<!-- prepush ([0-9a-z]{40}) -->");

private void showHelp(PrintWriter reply) {
reply.println("usage: `/integrate [auto|manual|<hash>]`");
Expand Down Expand Up @@ -110,6 +113,12 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}
}

Optional<Hash> prepushHash = checkForPrePushHash(bot, pr, scratchPath, allComments, "integrate");
if (prepushHash.isPresent()) {
markIntegratedAndClosed(pr, prepushHash.get(), reply);
return;
}

var problem = checkProblem(pr.checks(pr.headHash()), "jcheck", pr);
if (problem.isPresent()) {
reply.print("Your integration request cannot be fulfilled at this time, as ");
Expand All @@ -124,7 +133,6 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

// Run a final jcheck to ensure the change has been properly reviewed
var success = false;
erikj79 marked this conversation as resolved.
Show resolved Hide resolved
try (var integrationLock = IntegrationLock.create(pr, Duration.ofMinutes(10))) {
if (!integrationLock.isLocked()) {
log.severe("Unable to acquire the integration lock for " + pr.webUrl());
Expand All @@ -135,10 +143,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
// Now that we have the integration lock, refresh the PR metadata
pr = pr.repository().pullRequest(pr.id());

var path = scratchPath.resolve("integrate").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
Repository localRepo = materializeLocalRepo(bot, pr, scratchPath, "integrate");
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews(),
bot.confOverrideRepository().orElse(null),
bot.confOverrideName(),
Expand Down Expand Up @@ -196,20 +201,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
// Rebase and push it!
if (!localHash.equals(PullRequestUtils.targetHash(pr, localRepo))) {
var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace(), original);
addPrePushComment(pr, amendedHash, rebaseMessage.toString());
localRepo.push(amendedHash, pr.repository().url(), pr.targetRef());
Copy link
Member

Choose a reason for hiding this comment

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

If the push fails and the bot dies, will it still be retried after this fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's the idea at least, but I should write a test for it. If the push fails, the next time around, the prepush comment is found, but the hash is not found in the target, so the IntegrationCommand will continue as if nothing has happened.

success = true;

var finalRebaseMessage = rebaseMessage.toString();
if (!finalRebaseMessage.isBlank()) {
reply.println(rebaseMessage.toString());
}
reply.println("Pushed as commit " + amendedHash.hex() + ".");
reply.println();
reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
pr.removeLabel("ready");
pr.removeLabel("rfr");
markIntegratedAndClosed(pr, amendedHash, reply);
} else {
reply.print("Warning! Your commit did not result in any changes! ");
reply.println("No push attempt will be made.");
Expand All @@ -222,6 +216,71 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}
}

static Repository materializeLocalRepo(PullRequestBot bot, PullRequest pr, Path scratchPath, String subdir) throws IOException {
var path = scratchPath.resolve(subdir).resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
return PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
}

/**
* Checks if a prepush comment has been created already. This could happen if
* the bot got interrupted after pushing, but before finishing closing the PR
* and adding the final push comment.
*/
static Optional<Hash> checkForPrePushHash(PullRequestBot bot, PullRequest pr, Path scratchPath,
List<Comment> allComments, String subdir) {
var botUser = pr.repository().forge().currentUser();
var prePushHashes = allComments.stream()
.filter(c -> c.author().equals(botUser))
.map(Comment::body)
.map(PRE_PUSH_PATTERN::matcher)
.filter(Matcher::find)
.map(m -> m.group(1))
.collect(Collectors.toList());
if (!prePushHashes.isEmpty()) {
var path = scratchPath.resolve("integrate").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
try {
var localRepo = materializeLocalRepo(bot, pr, scratchPath, subdir);
for (String prePushHash : prePushHashes) {
Hash hash = new Hash(prePushHash);
if (PullRequestUtils.isAncestorOfTarget(localRepo, hash)) {
// A previous attempt at pushing this PR was successful, but didn't finish
// closing the PR
log.info("Found previous successful push in prepush comment: " + hash.hex());
return Optional.of(hash);
}
}
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
return Optional.empty();
}

static void addPrePushComment(PullRequest pr, Hash hash, String extraMessage) {
var commentBody = new StringWriter();
var writer = new PrintWriter(commentBody);
writer.println(PRE_PUSH_MARKER.formatted(hash.hex()));
writer.println("Going to push as commit " + hash.hex() + ".");
if (!extraMessage.isBlank()) {
writer.println(extraMessage);
}
pr.addComment(commentBody.toString());
}

static void markIntegratedAndClosed(PullRequest pr, Hash hash, PrintWriter reply) {
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
pr.removeLabel("ready");
pr.removeLabel("rfr");
reply.println("Pushed as commit " + hash.hex() + ".");
reply.println();
reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
}

@Override
public String description() {
return "performs integration of the changes in the PR";
Expand Down
5 changes: 4 additions & 1 deletion bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,10 @@ public Collection<WorkItem> run(Path scratchPath) {
var command = nextCommand.get();
log.info("Processing command: " + command.id() + " - " + command.name());

if (!pr.labelNames().contains("integrated")) {
// We can't trust just the integrated label as that gets set before the commit comment.
// If marked as integrated but there is no commit comment, any integrate command needs
// to run again to correct the state of the PR.
if (!pr.labelNames().contains("integrated") || resultingCommitHash(comments).isEmpty()) {
processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), command, comments, false);
// Must re-fetch PR after running the command, the command might have updated the PR
var updatedPR = pr.repository().pullRequest(pr.id());
Expand Down
32 changes: 15 additions & 17 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
return;
}

Optional<Hash> prePushHash = IntegrateCommand.checkForPrePushHash(bot, pr, scratchPath, allComments, "sponsor");
if (prePushHash.isPresent()) {
markIntegratedAndClosed(pr, prePushHash.get(), reply);
return;
}

var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments);
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.");
Expand Down Expand Up @@ -85,17 +91,13 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
// Now that we have the integration lock, refresh the PR metadata
pr = pr.repository().pullRequest(pr.id());

var path = scratchPath.resolve("sponsor").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var localRepo = IntegrateCommand.materializeLocalRepo(bot, pr, scratchPath, "sponsor");
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews(),
bot.confOverrideRepository().orElse(null),
bot.confOverrideName(),
bot.confOverrideRef());

// Validate the target hash if requested
var rebaseMessage = new StringWriter();
if (!command.args().isBlank()) {
var wantedHash = new Hash(command.args());
if (!PullRequestUtils.targetHash(pr, localRepo).equals(wantedHash)) {
Expand All @@ -106,6 +108,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

// Now rebase onto the target hash
var rebaseMessage = new StringWriter();
var rebaseWriter = new PrintWriter(rebaseMessage);
var rebasedHash = checkablePr.mergeTarget(rebaseWriter);
if (rebasedHash.isEmpty()) {
Expand Down Expand Up @@ -139,19 +142,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

if (!localHash.equals(PullRequestUtils.targetHash(pr, localRepo))) {
var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace(), original);
IntegrateCommand.addPrePushComment(pr, amendedHash, rebaseMessage.toString());
localRepo.push(amendedHash, pr.repository().url(), pr.targetRef());
var finalRebaseMessage = rebaseMessage.toString();
if (!finalRebaseMessage.isBlank()) {
reply.println(rebaseMessage.toString());
}
reply.println("Pushed as commit " + amendedHash.hex() + ".");
reply.println();
reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
pr.removeLabel("sponsor");
pr.removeLabel("ready");
pr.removeLabel("rfr");
markIntegratedAndClosed(pr, amendedHash, reply);
} else {
reply.print("Warning! This commit did not result in any changes! ");
reply.println("No push attempt will be made.");
Expand All @@ -164,6 +157,11 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}
}

private void markIntegratedAndClosed(PullRequest pr, Hash amendedHash, PrintWriter reply) {
IntegrateCommand.markIntegratedAndClosed(pr, amendedHash, reply);
pr.removeLabel("sponsor");
}

@Override
public String description() {
return "performs integration of a PR that is authored by a non-committer";
Expand Down
Loading