diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/prbranch/PullRequestBranchNotifier.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/prbranch/PullRequestBranchNotifier.java index 08d298b99..61b02e510 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/prbranch/PullRequestBranchNotifier.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/prbranch/PullRequestBranchNotifier.java @@ -77,7 +77,7 @@ public void onNewPullRequest(PullRequest pr, Path scratchPath) { @Override public void onStateChange(PullRequest pr, Path scratchPath, Issue.State oldState) { - if (pr.state() == Issue.State.CLOSED) { + if (pr.state() != Issue.State.OPEN) { var retargetedDependencies = PreIntegrations.retargetDependencies(pr); deleteBranch(pr); if (pr.labelNames().contains("integrated")) { diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java index 647ede33b..f26ea17c4 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java @@ -22,20 +22,33 @@ */ package org.openjdk.skara.bots.pr; -import org.openjdk.skara.forge.*; +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.io.UncheckedIOException; + 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; import java.time.Duration; -import java.util.*; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.logging.Level; -import java.util.stream.Collectors; import java.util.logging.Logger; -import java.util.regex.Pattern; import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + +import org.openjdk.skara.forge.Check; +import org.openjdk.skara.forge.CheckStatus; +import org.openjdk.skara.forge.CommitFailure; +import org.openjdk.skara.forge.HostedRepositoryPool; +import org.openjdk.skara.forge.PullRequest; +import org.openjdk.skara.forge.PullRequestUtils; public class IntegrateCommand implements CommandHandler { private final static Logger log = Logger.getLogger("org.openjdk.skara.bots.pr"); @@ -160,10 +173,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } - Optional prepushHash = checkForPrePushHash(bot, pr, scratchPath, allComments, "integrate"); - if (prepushHash.isPresent()) { - markIntegratedAndClosed(pr, prepushHash.get(), reply); - return; + // final Optional prepushHash = checkForPrePushHash(bot, pr, scratchPath, allComments, "integrate"); + + if (pr.state() == PullRequest.State.RESOLVED) { + markIntegratedAndMerge(pr, reply); } var problem = checkProblem(pr.checks(pr.headHash()), "jcheck", pr); @@ -187,10 +200,19 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } + final boolean integrationBranchExists = pr.repository().branchHash("integration/" + pr.id()).isPresent(); + // Now that we have the integration lock, refresh the PR metadata pr = pr.repository().pullRequest(pr.id()); Repository localRepo = materializeLocalRepo(bot, pr, scratchPath, "integrate"); + + if (integrationBranchExists) { + final Hash last = localRepo.fetch(pr.repository().url(), "+integration/" + pr.id() + ":integration/" + pr.id(), true); + localRepo.push(last, pr.sourceRepository().orElseThrow().url(), pr.sourceRef(), true); + pr = pr.repository().pullRequest(pr.id()); + } + var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews(), bot.confOverrideRepository().orElse(null), bot.confOverrideName(), @@ -207,6 +229,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst var rebaseWriter = new PrintWriter(rebaseMessage); var rebasedHash = checkablePr.mergeTarget(rebaseWriter); if (rebasedHash.isEmpty()) { + if (integrationBranchExists) { + pr.repository().deleteBranch("integration/" + pr.id()); + } reply.println(rebaseMessage); return; } @@ -239,9 +264,11 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst // Rebase and push it! if (!localHash.equals(checkablePr.targetHash())) { var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace(), original); + localRepo.push(amendedHash, pr.repository().url(), "integration/" + pr.id(), true); addPrePushComment(pr, amendedHash, rebaseMessage.toString()); - localRepo.push(amendedHash, pr.repository().url(), pr.targetRef()); - markIntegratedAndClosed(pr, amendedHash, reply); + // Force the Pull Request branch to accept edits from maintainers somehow? + localRepo.push(amendedHash, pr.sourceRepository().orElseThrow().url(), pr.sourceRef(), true); + markIntegratedAndMerge(pr, reply); } else { reply.print("Warning! Your commit did not result in any changes! "); reply.println("No push attempt will be made."); @@ -318,18 +345,21 @@ 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() + "."); + writer.println("Recording snapshot of final changes as commit " + hash.hex() + "."); + writer.println("New changes can no longer be made past this point, except for if an automatic rebase fails."); if (!extraMessage.isBlank()) { writer.println(extraMessage); } pr.addComment(commentBody.toString()); } - static void markIntegratedAndClosed(PullRequest pr, Hash hash, PrintWriter reply) { + static void markIntegratedAndMerge(PullRequest pr,PrintWriter reply) { // Note that the order of operations here is tested in IntegrateTests::retryAfterInterrupt // so any change here requires careful update of that test pr.addLabel("integrated"); - pr.setState(PullRequest.State.CLOSED); + if (pr.state() != PullRequest.State.RESOLVED) { + pr.setState(PullRequest.State.RESOLVED); + } pr.removeLabel("ready"); pr.removeLabel("rfr"); if (pr.labelNames().contains("delegated")) { @@ -338,9 +368,10 @@ static void markIntegratedAndClosed(PullRequest pr, Hash hash, PrintWriter reply if (pr.labelNames().contains("sponsor")) { pr.removeLabel("sponsor"); } - reply.println(PullRequest.commitHashMessage(hash)); + reply.println(PullRequest.commitHashMessage(pr.findIntegratedCommitHash().get())); reply.println(); - reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored."); + reply.println(":bulb: You may see a message that branches were created around your pull request. This can be safely ignored."); + pr.repository().deleteBranch("integration/" + pr.id()); } @Override diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java index d71c49a5f..8c92ed857 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java @@ -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; @@ -47,10 +48,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } - Optional prePushHash = IntegrateCommand.checkForPrePushHash(bot, pr, scratchPath, allComments, "sponsor"); - if (prePushHash.isPresent()) { - markIntegratedAndClosed(pr, prePushHash.get(), reply); - return; + // final Optional prePushHash = IntegrateCommand.checkForPrePushHash(bot, pr, scratchPath, allComments, "sponsor"); + + if (pr.state() == PullRequest.State.RESOLVED) { + IntegrateCommand.markIntegratedAndMerge(pr, reply); } var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments); @@ -91,10 +92,19 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } + final boolean integrationBranchExists = pr.repository().branchHash("integration/" + pr.id()).isPresent(); + // Now that we have the integration lock, refresh the PR metadata pr = pr.repository().pullRequest(pr.id()); - var localRepo = IntegrateCommand.materializeLocalRepo(bot, pr, scratchPath, "sponsor"); + Repository localRepo = IntegrateCommand.materializeLocalRepo(bot, pr, scratchPath, "sponsor"); + + if (integrationBranchExists) { + final Hash last = localRepo.fetch(pr.repository().url(), "+integration/" + pr.id() + ":integration/" + pr.id(), true); + localRepo.push(last, pr.sourceRepository().orElseThrow().url(), pr.sourceRef(), true); + pr = pr.repository().pullRequest(pr.id()); + } + var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews(), bot.confOverrideRepository().orElse(null), bot.confOverrideName(), @@ -115,6 +125,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst var rebaseWriter = new PrintWriter(rebaseMessage); var rebasedHash = checkablePr.mergeTarget(rebaseWriter); if (rebasedHash.isEmpty()) { + if (integrationBranchExists) { + pr.repository().deleteBranch("integration/" + pr.id()); + } reply.println(rebaseMessage); return; } @@ -129,9 +142,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst if (!localHash.equals(checkablePr.targetHash())) { var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace(), original); + localRepo.push(amendedHash, pr.repository().url(), "integration/" + pr.id(), true); IntegrateCommand.addPrePushComment(pr, amendedHash, rebaseMessage.toString()); - localRepo.push(amendedHash, pr.repository().url(), pr.targetRef()); - markIntegratedAndClosed(pr, amendedHash, reply); + localRepo.push(amendedHash, pr.sourceRepository().orElseThrow().url(), pr.sourceRef(), true); + IntegrateCommand.markIntegratedAndMerge(pr, reply); } else { reply.print("Warning! This commit did not result in any changes! "); reply.println("No push attempt will be made."); @@ -144,10 +158,6 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } } - private void markIntegratedAndClosed(PullRequest pr, Hash amendedHash, PrintWriter reply) { - IntegrateCommand.markIntegratedAndClosed(pr, amendedHash, reply); - } - @Override public String description() { return "performs integration of a PR that is authored by a non-committer"; diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java index 956f42c44..b6c628120 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java @@ -531,7 +531,7 @@ void autoRebase(TestInfo testInfo) throws IOException { // The bot should reply with an ok message var prePush = pr.comments().stream() - .filter(comment -> comment.body().contains("Going to push as commit")) + .filter(comment -> comment.body().contains("Recording snapshot of final changes as commit")) .filter(comment -> comment.body().contains("commit was automatically rebased without conflicts")) .count(); assertEquals(1, prePush); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java index 078ec9cc4..639ba3549 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SponsorTests.java @@ -372,7 +372,7 @@ void autoRebase(TestInfo testInfo) throws IOException { // The bot should reply with an ok message var prePush = pr.comments().stream() - .filter(comment -> comment.body().contains("Going to push as commit")) + .filter(comment -> comment.body().contains("Recording snapshot of final changes as commit")) .filter(comment -> comment.body().contains("commit was automatically rebased without conflicts")) .count(); assertEquals(1, prePush); diff --git a/forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java b/forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java index 49d1febe2..0a0ba4a29 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java +++ b/forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java @@ -84,7 +84,7 @@ public HostUser author() { @Override public List reviews() { var currentTargetRef = targetRef(); - var reviews = request.get("pulls/" + json.get("number").toString() + "/reviews").execute().stream() + var reviews = request.get("pulls/" + this.id() + "/reviews").execute().stream() .map(JSONValue::asObject) .filter(obj -> !(obj.get("state").asString().equals("COMMENTED") && obj.get("body").asString().isEmpty())) .map(obj -> { @@ -172,14 +172,14 @@ public void addReview(Review.Verdict verdict, String body) { } query.put("commit_id", headHash().hex()); query.put("comments", JSON.array()); - request.post("pulls/" + json.get("number").toString() + "/reviews") + request.post("pulls/" + this.id() + "/reviews") .body(query) .execute(); } @Override public void updateReview(int id, String body) { - request.put("pulls/" + json.get("number").toString() + "/reviews/" + id) + request.put("pulls/" + this.id() + "/reviews/" + id) .body("body", body) .execute(); } @@ -240,7 +240,7 @@ public ReviewComment addReviewComment(Hash base, Hash hash, String path, int lin .put("path", path) .put("side", "RIGHT") .put("line", line); - var response = request.post("pulls/" + json.get("number").toString() + "/comments") + var response = request.post("pulls/" + this.id() + "/comments") .body(query) .execute(); return parseReviewComment(null, response.asObject()); @@ -251,7 +251,7 @@ public ReviewComment addReviewCommentReply(ReviewComment parent, String body) { var query = JSON.object() .put("body", body) .put("in_reply_to", Integer.parseInt(parent.threadId())); - var response = request.post("pulls/" + json.get("number").toString() + "/comments") + var response = request.post("pulls/" + this.id() + "/comments") .body(query) .execute(); return parseReviewComment(parent, response.asObject()); @@ -260,7 +260,7 @@ public ReviewComment addReviewCommentReply(ReviewComment parent, String body) { @Override public List reviewComments() { var ret = new ArrayList(); - var reviewComments = request.get("pulls/" + json.get("number").toString() + "/comments").execute().stream() + var reviewComments = request.get("pulls/" + this.id() + "/comments").execute().stream() .map(JSONValue::asObject) .collect(Collectors.toList()); var idToComment = new HashMap(); @@ -314,7 +314,7 @@ public String title() { @Override public void setTitle(String title) { - request.patch("pulls/" + json.get("number").toString()) + request.patch("pulls/" + this.id()) .body("title", title) .execute(); } @@ -330,7 +330,7 @@ public String body() { @Override public void setBody(String body) { - request.patch("pulls/" + json.get("number").toString()) + request.patch("pulls/" + this.id()) .body("body", body) .execute(); } @@ -346,7 +346,7 @@ private Comment parseComment(JSONValue comment) { @Override public List comments() { - return request.get("issues/" + json.get("number").toString() + "/comments").execute().stream() + return request.get("issues/" + this.id() + "/comments").execute().stream() .map(this::parseComment) .collect(Collectors.toList()); } @@ -354,7 +354,7 @@ public List comments() { @Override public Comment addComment(String body) { body = limitBodySize(body); - var comment = request.post("issues/" + json.get("number").toString() + "/comments") + var comment = request.post("issues/" + this.id() + "/comments") .body("body", body) .execute(); return parseComment(comment); @@ -398,10 +398,36 @@ public ZonedDateTime updatedAt() { @Override public State state() { - if (json.get("state").asString().equals("open")) { - return State.OPEN; + return json.get("state").asString().equals("open") ? State.OPEN : + (json.get("merged_at").isNull() ? State.CLOSED : State.RESOLVED); + } + + @Override + public void setState(State state) { + if (state == State.RESOLVED) { + + /* + * + * SKARA-1663: Implement State.RESOLVED as a rebase for GitHub, until GitHub + * allows for marking a Pull Request as merged in the future. + * + * If a method for actual merging is required, the following implementation + * will suffice for the different types of merges GitHub allows for: + * + * request.put("pulls/" + this.id() + "/merge").body("merge_method", "merge") + * .execute(); + * + * Where "merge_method" can be set to one of: "rebase", "squash", or "merge" + */ + + request.put("pulls/" + this.id() + "/merge") + .body("merge_method", "rebase") + .execute(); + } else { + request.patch("pulls/" + this.id()) + .body("state", state == State.OPEN ? "open" : "closed") + .execute(); } - return State.CLOSED; } @Override @@ -527,18 +553,11 @@ public boolean isDraft() { return json.get("draft").asBoolean(); } - @Override - public void setState(State state) { - request.patch("pulls/" + json.get("number").toString()) - .body("state", state != State.OPEN ? "closed" : "open") - .execute(); - } - @Override public void addLabel(String label) { labels = null; var query = JSON.object().put("labels", JSON.array().add(label)); - request.post("issues/" + json.get("number").toString() + "/labels") + request.post("issues/" + this.id() + "/labels") .body(query) .execute(); } @@ -546,7 +565,7 @@ public void addLabel(String label) { @Override public void removeLabel(String label) { labels = null; - request.delete("issues/" + json.get("number").toString() + "/labels/" + label) + request.delete("issues/" + this.id() + "/labels/" + label) .onError(r -> { // The GitHub API explicitly states that 404 is the response for deleting labels currently not set if (r.statusCode() == 404) { @@ -564,7 +583,7 @@ public void setLabels(List labels) { labelArray.add(label); } var query = JSON.object().put("labels", labelArray); - var newLabels = request.put("issues/" + json.get("number").toString() + "/labels") + var newLabels = request.put("issues/" + this.id() + "/labels") .body(query) .execute() .stream() @@ -576,7 +595,7 @@ public void setLabels(List labels) { @Override public List