diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java index f33016092..47c442619 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java @@ -40,13 +40,13 @@ import java.util.logging.Logger; import java.util.regex.Pattern; import java.util.stream.Collectors; -import java.util.stream.Stream; class CheckWorkItem extends PullRequestWorkItem { private final Pattern metadataComments = Pattern.compile("\n" + + ":warning: @" + pr.author().username() + " the issue prefix `" + prefix + "` does not" + + " match project [" + project.name() + "](" + project.webUrl() + ")."; + addBackportErrorComment(text, comments); + return List.of(); + } + var issue = project.issue(id); + if (issue.isEmpty()) { + var text = "\n" + + ":warning: @" + pr.author().username() + " the issue with id `" + id + "` " + + "does not exist in project [" + project.name() + "](" + project.webUrl() + ")."; + addBackportErrorComment(text, comments); + return List.of(); + } + pr.setTitle(id + ": " + issue.get().title()); + var text = "This backport pull request has now been updated with the original issue," + + " but not the original commit. If you have the original commit hash, please update" + + " the pull request title with `Backport `."; + pr.addComment(text); + pr.addLabel("backport"); + return List.of(new CheckWorkItem(bot, pr.repository().pullRequest(pr.id()), errorHandler)); + } + // If the title needs updating, we run the check again if (updateTitle()) { return List.of(new CheckWorkItem(bot, pr.repository().pullRequest(pr.id()), errorHandler)); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java index f331a0c22..f24feb6ea 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java @@ -266,6 +266,10 @@ Optional mergeTarget(PrintWriter reply) { } Hash findOriginalBackportHash() { + return findOriginalBackportHash(pr); + } + + static Hash findOriginalBackportHash(PullRequest pr) { var botUser = pr.repository().forge().currentUser(); var backportLines = pr.comments() .stream() diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CleanCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CleanCommand.java index 5875e7431..c91f4156d 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CleanCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CleanCommand.java @@ -61,8 +61,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } - if (!pr.labelNames().contains("backport")) { - reply.println("@" + username + " can only mark [backport pull requests](https://wiki.openjdk.java.net/display/SKARA/Backports#Backports-BackportPullRequests) as clean"); + if (!pr.labelNames().contains("backport") || CheckablePullRequest.findOriginalBackportHash(pr) == null) { + reply.println("@" + username + " can only mark [backport pull requests]" + + "(https://wiki.openjdk.java.net/display/SKARA/Backports#Backports-BackportPullRequests)," + + " with an original hash, as clean"); return; } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/BackportTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/BackportTests.java index e7411fcac..b47de3277 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/BackportTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/BackportTests.java @@ -1347,4 +1347,116 @@ void badIssueInOriginal(TestInfo testInfo) throws IOException { assertFalse(pr.labelNames().contains("backport")); } } + + @Test + void noHashOnlyIssue(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + var reviewer = credentials.getHostedRepository(); + var issues = credentials.getIssueProject(); + var censusBuilder = credentials.getCensusBuilder() + .addCommitter(author.forge().currentUser().id()) + .addReviewer(integrator.forge().currentUser().id()) + .addReviewer(reviewer.forge().currentUser().id()); + var bot = PullRequestBot.newBuilder() + .repo(integrator) + .censusRepo(censusBuilder.build()) + .issueProject(issues) + .build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + localRepo.push(masterHash, author.url(), "master", true); + + var issue1 = credentials.createIssue(issues, "An issue"); + var issue1Number = issue1.id().split("-")[1]; + + // Create change + localRepo.checkout(localRepo.defaultBranch()); + var editBranch = localRepo.branch(masterHash, "edit"); + localRepo.checkout(editBranch); + var newFile2 = localRepo.root().resolve("a_new_file.txt"); + Files.writeString(newFile2, "hello"); + localRepo.add(newFile2); + var editHash = localRepo.commit("Backport", "duke", "duke@openjdk.java.net"); + localRepo.push(editHash, author.url(), "refs/heads/edit", true); + + // Create various kinds of bad pull request titles + // Use a bad project + var pr = credentials.createPullRequest(author, "master", "edit", + "Backport " + "FOO-" + issue1.id().split("-")[1]); + TestBotRunner.runPeriodicItems(bot); + var backportComment = pr.comments().get(0).body(); + assertTrue(backportComment.contains("does not match project")); + assertFalse(pr.labelNames().contains("backport")); + + // Use bad issue ID + pr.setTitle("Backport TEST-4711"); + TestBotRunner.runPeriodicItems(bot); + backportComment = pr.comments().get(1).body(); + assertTrue(backportComment.contains("does not exist in project")); + assertFalse(pr.labelNames().contains("backport")); + + // Use different kinds of good titles + // Use the full issue ID + pr.setTitle("Backport " + issue1.id()); + TestBotRunner.runPeriodicItems(bot); + backportComment = pr.comments().get(2).body(); + assertTrue(backportComment.contains("This backport pull request has now been updated with the original issue")); + assertEquals(issue1Number + ": An issue", pr.title()); + assertTrue(pr.labelNames().contains("backport")); + + // Set the title without project name + pr.setTitle("Backport " + issue1.id().split("-")[1]); + TestBotRunner.runPeriodicItems(bot); + backportComment = pr.comments().get(3).body(); + assertTrue(backportComment.contains("This backport pull request has now been updated with the original issue")); + assertEquals(issue1Number + ": An issue", pr.title()); + assertTrue(pr.labelNames().contains("backport")); + + // Approve PR and re-run bot + var prAsReviewer = reviewer.pullRequest(pr.id()); + prAsReviewer.addReview(Review.Verdict.APPROVED, "Looks good"); + TestBotRunner.runPeriodicItems(bot); + assertLastCommentContains(pr, "This change now passes all *automated* pre-integration checks"); + + // Integrate + var prAsCommitter = author.pullRequest(pr.id()); + prAsCommitter.addComment("/integrate"); + TestBotRunner.runPeriodicItems(bot); + + // Find the commit + assertLastCommentContains(pr, "Pushed as commit"); + + String hex = null; + var comment = pr.comments().get(pr.comments().size() - 1); + var lines = comment.body().split("\n"); + var pattern = Pattern.compile(".* Pushed as commit ([0-9a-z]{40}).*"); + for (var line : lines) { + var m = pattern.matcher(line); + if (m.matches()) { + hex = m.group(1); + break; + } + } + assertNotNull(hex); + assertEquals(40, hex.length()); + localRepo.checkout(localRepo.defaultBranch()); + localRepo.pull(author.url().toString(), "master", false); + var commit = localRepo.lookup(new Hash(hex)).orElseThrow(); + + var message = CommitMessageParsers.v1.parse(commit); + assertEquals(1, message.issues().size()); + assertEquals("An issue", message.issues().get(0).description()); + assertEquals(List.of("integrationreviewer3"), message.reviewers()); + assertEquals(Optional.empty(), message.original()); + assertEquals(List.of(), message.contributors()); + assertEquals(List.of(), message.summaries()); + assertEquals(List.of(), message.additional()); + } + } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CleanCommandTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CleanCommandTests.java index 2f20e2909..a530eef61 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CleanCommandTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CleanCommandTests.java @@ -73,7 +73,7 @@ void cleanCommandOnRegularPullRequestShouldNotWork(TestInfo testInfo) throws IOE assertFalse(pr.labelNames().contains("backport")); assertFalse(pr.labelNames().contains("clean")); assertLastCommentContains(pr, "can only mark [backport pull requests]"); - assertLastCommentContains(pr, "as clean"); + assertLastCommentContains(pr, ", with an original hash, as clean"); } } @@ -297,4 +297,49 @@ void authorShouldNotBeAllowed(TestInfo testInfo) throws IOException { assertLastCommentContains(pr, "can use the `/clean` command"); } } + + @Test + void missingBackportHash(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addCommitter(author.forge().currentUser().id()) + .addReviewer(integrator.forge().currentUser().id()); + var issues = credentials.getIssueProject(); + var prBot = PullRequestBot.newBuilder() + .repo(integrator) + .censusRepo(censusBuilder.build()) + .issueProject(issues) + .build(); + + // Populate the projects repository + var localRepoFolder = tempFolder.path().resolve("localrepo"); + var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + var issue = credentials.createIssue(issues, "An issue"); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "Backport " + issue.id()); + TestBotRunner.runPeriodicItems(prBot); + + assertTrue(pr.labelNames().contains("backport")); + assertFalse(pr.labelNames().contains("clean")); + + // Try to issue the "/clean" PR command, should not work + pr.addComment("/clean"); + TestBotRunner.runPeriodicItems(prBot); + assertTrue(pr.labelNames().contains("backport")); + assertFalse(pr.labelNames().contains("clean")); + assertLastCommentContains(pr, "can only mark [backport pull requests]"); + assertLastCommentContains(pr, ", with an original hash, as clean"); + } + } }