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 5705f0214..cd4139312 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 @@ -42,8 +42,15 @@ public class IntegrateCommand implements CommandHandler { private static final String PRE_PUSH_MARKER = ""; private static final Pattern PRE_PUSH_PATTERN = Pattern.compile(""); + private enum Command { + auto, + manual, + defer, + undefer + } + private void showHelp(PrintWriter reply) { - reply.println("usage: `/integrate [auto|manual|]`"); + reply.println("usage: `/integrate [auto|manual|defer|undefer|]`"); } private Optional checkProblem(Map performedChecks, String checkName, PullRequest pr) { @@ -66,22 +73,9 @@ private Optional checkProblem(Map performedChecks, String @Override public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { - if (!command.user().equals(pr.author()) && !command.user().equals(pr.repository().forge().currentUser())) { - reply.print("Only the author (@" + pr.author().username() + ") is allowed to issue the `integrate` command."); - - // If the command author is allowed to sponsor this change, suggest that command - var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments); - if (readyHash.isPresent()) { - if (censusInstance.isCommitter(command.user())) { - reply.print(" As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the `/sponsor` command?"); - return; - } - } - reply.println(); - return; - } - + // Parse any argument given Hash targetHash = null; + Command commandArg = null; if (!command.args().isEmpty()) { var args = command.args().split(" "); if (args.length != 1) { @@ -90,19 +84,12 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } var arg = args[0].trim(); - if (arg.equals("auto")) { - pr.addLabel("auto"); - reply.println("This pull request will be automatically integrated when it is ready"); - return; - } else if (arg.equals("manual")) { - if (pr.labelNames().contains("auto")) { - pr.removeLabel("auto"); + for (Command value : Command.values()) { + if (value.name().equals(arg)) { + commandArg = value; } - reply.println("This pull request will have to be integrated manually using the "+ - "[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command."); - return; - } else { - // Validate the target hash if requested + } + if (commandArg == null) { targetHash = new Hash(arg); if (!targetHash.isValid()) { reply.println("The given argument, `" + arg + "`, is not a valid hash."); @@ -111,6 +98,60 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } } + if (!command.user().equals(pr.author()) && !command.user().equals(pr.repository().forge().currentUser())) { + if (pr.labelNames().contains("deferred")) { + // Check that the command user is a committer + if (!censusInstance.isCommitter(command.user())) { + reply.print("Only project committers are allowed to issue the `integrate` command on a deferred pull request."); + return; + } + // Check that no extra arguments are added + if (!command.args().isEmpty()) { + reply.print("Only the author (@\" + pr.author().username() + \") is allowed to issue the `integrate` command with arguments."); + return; + } + } else { + reply.print("Only the author (@" + pr.author().username() + ") is allowed to issue the `integrate` command."); + + // If the command author is allowed to sponsor this change, suggest that command + var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments); + if (readyHash.isPresent()) { + if (censusInstance.isCommitter(command.user())) { + reply.print(" As this pull request is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the `/sponsor` command?"); + return; + } + } + reply.println(); + return; + } + } + + if (commandArg == Command.auto) { + pr.addLabel("auto"); + reply.println("This pull request will be automatically integrated when it is ready"); + return; + } else if (commandArg == Command.manual) { + if (pr.labelNames().contains("auto")) { + pr.removeLabel("auto"); + } + reply.println("This pull request will have to be integrated manually using the " + + "[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command."); + return; + } else if (commandArg == Command.defer) { + pr.addLabel("deferred"); + reply.println("Integration of this pull request has been deferred and may be completed by any project committer using the " + + "[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command."); + return; + } else if (commandArg == Command.undefer) { + if (pr.labelNames().contains("deferred")) { + reply.println("Integration of this pull request is no longer deferred and may only be integrated by the author (@" + pr.author().username() + ")using the " + + "[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command."); + pr.removeLabel("deferred"); + } + reply.println("This pull request may now only be integrated by the author"); + return; + } + Optional prepushHash = checkForPrePushHash(bot, pr, scratchPath, allComments, "integrate"); if (prepushHash.isPresent()) { markIntegratedAndClosed(pr, prepushHash.get(), reply); @@ -126,7 +167,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst var labels = new HashSet<>(pr.labelNames()); if (!labels.contains("ready")) { - reply.println("This PR has not yet been marked as ready for integration."); + reply.println("This pull request has not yet been marked as ready for integration."); return; } @@ -163,7 +204,14 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } var original = checkablePr.findOriginalBackportHash(); - var localHash = checkablePr.commit(rebasedHash.get(), censusInstance.namespace(), censusInstance.configuration().census().domain(), null, original); + // If someone other than the author or the bot issued the /integrate command, then that person + // should be set as sponsor/integrator. Otherwise pass null to use the default author. + String committerId = null; + if (!command.user().equals(pr.author()) && !command.user().equals(pr.repository().forge().currentUser())) { + committerId = command.user().id(); + } + var localHash = checkablePr.commit(rebasedHash.get(), censusInstance.namespace(), + censusInstance.configuration().census().domain(), committerId, original); if (runJcheck(pr, censusInstance, allComments, reply, localRepo, checkablePr, localHash)) { return; @@ -274,6 +322,9 @@ static void markIntegratedAndClosed(PullRequest pr, Hash hash, PrintWriter reply pr.addLabel("integrated"); pr.removeLabel("ready"); pr.removeLabel("rfr"); + if (pr.labelNames().contains("deferred")) { + pr.removeLabel("deferred"); + } 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."); 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 6da641d99..948a2051f 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 @@ -44,14 +44,13 @@ void simpleMerge(TestInfo testInfo) throws IOException { var tempFolder = new TemporaryDirectory(); var pushedFolder = new TemporaryDirectory()) { + var botUser = credentials.getHostedRepository(); var author = credentials.getHostedRepository(); - var integrator = credentials.getHostedRepository(); var reviewer = credentials.getHostedRepository(); var censusBuilder = credentials.getCensusBuilder() .addCommitter(author.forge().currentUser().id()) - .addReviewer(integrator.forge().currentUser().id()) .addReviewer(reviewer.forge().currentUser().id()); - var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build(); + var mergeBot = PullRequestBot.newBuilder().repo(botUser).censusRepo(censusBuilder.build()).build(); // Populate the projects repository var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); @@ -65,8 +64,8 @@ void simpleMerge(TestInfo testInfo) throws IOException { var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); // Approve it as another user - var approvalPr = integrator.pullRequest(pr.id()); - approvalPr.addReview(Review.Verdict.APPROVED, "Approved"); + var reviewerPr = reviewer.pullRequest(pr.id()); + reviewerPr.addReview(Review.Verdict.APPROVED, "Approved"); // The bot should reply with integration message TestBotRunner.runPeriodicItems(mergeBot); @@ -234,7 +233,7 @@ void notReviewed(TestInfo testInfo) throws IOException { TestBotRunner.runPeriodicItems(mergeBot); // The bot should reply with an error message - assertLastCommentContains(pr, "PR has not yet been marked as ready for integration"); + assertLastCommentContains(pr, "pull request has not yet been marked as ready for integration"); } } @@ -651,7 +650,7 @@ void cannotRebase(TestInfo testInfo) throws IOException { // The bot should reply with an error message TestBotRunner.runPeriodicItems(mergeBot); - assertLastCommentContains(pr, "PR has not yet been marked as ready for integration"); + assertLastCommentContains(pr, "pull request has not yet been marked as ready for integration"); } } @@ -1296,4 +1295,113 @@ void retryAfterInterruptExtraChange(TestInfo testInfo) throws IOException { assertTrue(pr.labelNames().contains("integrated"), "integrated label not added"); } } + + @Test + void defer(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory(); + var pushedFolder = new TemporaryDirectory()) { + + var botUser = credentials.getHostedRepository(); + var author = credentials.getHostedRepository(); + var reviewer = credentials.getHostedRepository(); + var badIntegrator = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + var censusBuilder = credentials.getCensusBuilder() + .addCommitter(author.forge().currentUser().id()) + .addReviewer(reviewer.forge().currentUser().id()) + .addAuthor(badIntegrator.forge().currentUser().id()) + .addCommitter(integrator.forge().currentUser().id()); + var mergeBot = PullRequestBot.newBuilder().repo(botUser).censusRepo(censusBuilder.build()).build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "refs/heads/edit", true); + var authorPr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); + + // Approve it as another user + var reviewerPr = reviewer.pullRequest(authorPr.id()); + reviewerPr.addReview(Review.Verdict.APPROVED, "Approved"); + + // Issue /integrate defer command and verify the PR gets deferred + authorPr.addComment("/integrate defer"); + TestBotRunner.runPeriodicItems(mergeBot); + var deferred = authorPr.comments().stream() + .filter(comment -> comment.body().contains("Integration of this pull request has been deferred")) + .count(); + assertEquals(1, deferred, "Missing deferred message"); + assertTrue(authorPr.labelNames().contains("deferred")); + + // Try to integrate by non committer + var badIntegratorPr = badIntegrator.pullRequest(authorPr.id()); + badIntegratorPr.addComment("/integrate"); + TestBotRunner.runPeriodicItems(mergeBot); + var onlyCommitters = authorPr.comments().stream() + .filter(comment -> comment.body() + .contains("Only project committers are allowed to issue the `integrate` command on a deferred pull request.")) + .count(); + assertEquals(1, onlyCommitters, "Missing error about only committers can integrate"); + + // Issue /integrate undefer and verify the PR is no longer deferred + authorPr.addComment("/integrate undefer"); + TestBotRunner.runPeriodicItems(mergeBot); + var undeferred = authorPr.comments().stream() + .filter(comment -> comment.body().contains("Integration of this pull request is no longer deferred and may only be integrated by the author")) + .count(); + assertEquals(1, undeferred, "Missing undeferred message"); + assertFalse(authorPr.labelNames().contains("deferred")); + + // Try integrating as another committer, which should fail since the PR is currently not deferred + var integratorPr = integrator.pullRequest(authorPr.id()); + integratorPr.addComment("/integrate"); + TestBotRunner.runPeriodicItems(mergeBot); + var nonAuthor = authorPr.comments().stream() + .filter(comment -> comment.body().contains("Only the author") + && comment.body().contains("is allowed to issue the `integrate` command")) + .count(); + assertEquals(1, nonAuthor, "Missing only author can integrate message"); + + // Defer again + authorPr.addComment("/integrate defer"); + TestBotRunner.runPeriodicItems(mergeBot); + assertTrue(authorPr.labelNames().contains("deferred")); + + // Try to issue /integrate with an invalid command for a non author + integratorPr.addComment("/integrate auto"); + TestBotRunner.runPeriodicItems(mergeBot); + var invalid = authorPr.comments().stream() + .filter(comment -> comment.body().contains("Only the author")) + .count(); + assertEquals(2, invalid, "Missing error message"); + + // Try to integrate by committer + integratorPr.addComment("/integrate"); + TestBotRunner.runPeriodicItems(mergeBot); + + // The change should now be present on the master branch + var pushedRepo = Repository.materialize(pushedFolder.path(), author.url(), "master"); + assertTrue(CheckableRepository.hasBeenEdited(pushedRepo)); + + var headHash = pushedRepo.resolve("HEAD").orElseThrow(); + var headCommit = pushedRepo.commits(headHash.hex() + "^.." + headHash.hex()).asList().get(0); + + // Verify that the author and committer of the change are the correct users + // The number is implied from the order the add* methods of CensusBuilder were called above. + assertEquals("Generated Committer 1", headCommit.author().name()); + assertEquals("integrationcommitter1@openjdk.java.net", headCommit.author().email()); + assertEquals("Generated Committer 4", headCommit.committer().name()); + assertEquals("integrationcommitter4@openjdk.java.net", headCommit.committer().email()); + assertTrue(authorPr.labelNames().contains("integrated")); + + // Ready and deferred labels should have been removed + assertFalse(authorPr.labelNames().contains("ready")); + assertFalse(authorPr.labelNames().contains("deferred")); + } + } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/PreIntegrateTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/PreIntegrateTests.java index f460e3083..aaf743e78 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/PreIntegrateTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/PreIntegrateTests.java @@ -92,7 +92,7 @@ void integrateFollowup(TestInfo testInfo) throws IOException { // Try to integrate it followUpPr.addComment("/integrate"); TestBotRunner.runPeriodicItems(mergeBot); - assertLastCommentContains(followUpPr, "This PR has not yet been marked as ready for integration"); + assertLastCommentContains(followUpPr, "This pull request has not yet been marked as ready for integration"); // Push something unrelated to the target localRepo.checkout(masterHash, true); diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java index fec95767b..c02d52193 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java @@ -179,7 +179,7 @@ void noIntegration(TestInfo testInfo) throws IOException { // It should not be possible to integrate yet pr.addComment("/integrate"); TestBotRunner.runPeriodicItems(prBot); - assertLastCommentContains(reviewerPr,"PR has not yet been marked as ready for integration"); + assertLastCommentContains(reviewerPr,"pull request has not yet been marked as ready for integration"); // Relax the requirement reviewerPr.addComment("/reviewers 1"); diff --git a/test/src/main/java/org/openjdk/skara/test/HostCredentials.java b/test/src/main/java/org/openjdk/skara/test/HostCredentials.java index 571160d50..d18a3001b 100644 --- a/test/src/main/java/org/openjdk/skara/test/HostCredentials.java +++ b/test/src/main/java/org/openjdk/skara/test/HostCredentials.java @@ -187,7 +187,8 @@ private static class TestCredentials implements Credentials { HostUser.create(1, "user1", "User Number 1"), HostUser.create(2, "user2", "User Number 2"), HostUser.create(3, "user3", "User Number 3"), - HostUser.create(4, "user4", "User Number 4") + HostUser.create(4, "user4", "User Number 4"), + HostUser.create(5, "user5", "User Number 5") ); private TestHost createHost(int userIndex) {