Skip to content
Permalink
Browse files

665: Mismatch between issue and PR title should not allow integration

Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Sep 24, 2020
1 parent 978cf2d commit c96307518e4f47af99fb616c41be40158d65de9e
@@ -179,6 +179,41 @@ private IssueProject issueProject() {
return ret;
}

private Map<String, String> blockingIntegrationLabels() {
return Map.of("rejected", "The change is currently blocked from integration by a rejection.",
"csr", "The change requires a CSR request to be approved.");
}

private List<String> botSpecificIntegrationBlockers() {
var ret = new ArrayList<String>();

var issues = issues();
var issueProject = issueProject();
if (issueProject != null) {
for (var currentIssue : issues) {
try {
var iss = issueProject.issue(currentIssue.shortId());
if (iss.isPresent()) {
if (!relaxedEquals(iss.get().title(), currentIssue.description())) {
var issueString = "[" + iss.get().id() + "](" + iss.get().webUrl() + ")";
ret.add("Title mismatch between PR and JBS for issue " + issueString);
}
} else {
log.warning("Failed to retrieve information on issue " + currentIssue.id());
}
} catch (RuntimeException e) {
log.warning("Temporary failure when trying to retrieve information on issue " + currentIssue.id());
}
}
}

labels.stream()
.filter(l -> blockingIntegrationLabels().containsKey(l))
.forEach(l -> ret.add(blockingIntegrationLabels().get(l)));

return ret;
}

private void updateCheckBuilder(CheckBuilder checkBuilder, PullRequestCheckIssueVisitor visitor, List<String> additionalErrors) {
if (visitor.isReadyForReview() && additionalErrors.isEmpty()) {
checkBuilder.complete(true);
@@ -263,7 +298,7 @@ private String getChecksList(PullRequestCheckIssueVisitor visitor) {
.collect(Collectors.joining("\n"));
}

private String getAdditionalErrorsList(List<String> additionalErrors) {
private String warningListToText(List<String> additionalErrors) {
return additionalErrors.stream()
.sorted()
.map(err -> "&nbsp;⚠️ " + err)
@@ -334,7 +369,8 @@ private boolean relaxedEquals(String s1, String s2) {
}


private String getStatusMessage(List<Comment> comments, List<Review> reviews, PullRequestCheckIssueVisitor visitor, List<String> additionalErrors) {
private String getStatusMessage(List<Comment> comments, List<Review> reviews, PullRequestCheckIssueVisitor visitor,
List<String> additionalErrors, List<String> integrationBlockers) {
var progressBody = new StringBuilder();
progressBody.append("---------\n");
progressBody.append("### Progress\n");
@@ -349,7 +385,16 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
progressBody.append("s");
}
progressBody.append("\n");
progressBody.append(getAdditionalErrorsList(allAdditionalErrors));
progressBody.append(warningListToText(allAdditionalErrors));
}

if (!integrationBlockers.isEmpty()) {
progressBody.append("\n\n### Integration blocker");
if (integrationBlockers.size() > 1) {
progressBody.append("s");
}
progressBody.append("\n");
progressBody.append(warningListToText(integrationBlockers));
}

var issues = issues();
@@ -721,8 +766,10 @@ private void checkStatus() {
updateCheckBuilder(checkBuilder, visitor, additionalErrors);
updateReadyForReview(visitor, additionalErrors);

var integrationBlockers = botSpecificIntegrationBlockers();

// Calculate and update the status message if needed
var statusMessage = getStatusMessage(comments, activeReviews, visitor, additionalErrors);
var statusMessage = getStatusMessage(comments, activeReviews, visitor, additionalErrors, integrationBlockers);
var updatedBody = updateStatusMessage(statusMessage);
var title = pr.title();

@@ -736,7 +783,7 @@ private void checkStatus() {
var commitMessage = String.join("\n", commit.message());
var readyForIntegration = visitor.messages().isEmpty() &&
additionalErrors.isEmpty() &&
labels.stream().noneMatch(l -> workItem.bot.blockingReadyLabels().contains(l));
integrationBlockers.isEmpty();

updateMergeReadyComment(readyForIntegration, commitMessage, comments, activeReviews, rebasePossible);
if (readyForIntegration && rebasePossible) {
@@ -78,11 +78,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

var labels = new HashSet<>(pr.labels());
for (var blocker : bot.blockingIntegrationLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
reply.println(blocker.getValue());
return;
}
if (!labels.contains("ready")) {
reply.println("This PR has not yet been marked as ready for integration.");
return;
}

// Run a final jcheck to ensure the change has been properly reviewed
@@ -172,22 +172,6 @@ LabelConfiguration labelConfiguration() {
return blockingCheckLabels;
}

Set<String> blockingReadyLabels() {
return Set.of("csr");
}

Map<String, String> blockingIntegrationLabels() {
return Map.of("rejected", "The change is currently blocked from integration by a rejection.");
}

Set<String> readyLabels() {
return readyLabels;
}

Map<String, Pattern> readyComments() {
return readyComments;
}

IssueProject issueProject() {
return issueProject;
}
@@ -60,11 +60,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

var labels = new HashSet<>(pr.labels());
for (var blocker : bot.blockingIntegrationLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
reply.println(blocker.getValue());
return;
}
if (!labels.contains("ready")) {
reply.println("This PR has not yet been marked as ready for integration.");
return;
}

// Notify the author as well
@@ -22,15 +22,11 @@
*/
package org.openjdk.skara.bots.pr;

import org.junit.jupiter.api.*;
import org.openjdk.skara.forge.Review;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.issuetracker.Link;
import org.openjdk.skara.issuetracker.Issue;
import org.openjdk.skara.test.*;
import org.openjdk.skara.vcs.Repository;
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.json.JSON;

import org.junit.jupiter.api.*;
import org.openjdk.skara.test.*;

import java.io.IOException;
import java.util.*;
@@ -527,6 +523,9 @@ void csrLabelShouldBlockReadyLabel(TestInfo testInfo) throws IOException {
// PR should not be ready
prAsAuthor = author.pullRequest(pr.id());
assertFalse(prAsAuthor.labels().contains("ready"));

// The body should contain a note about why
assertTrue(pr.body().contains("change requires a CSR request to be approved"));
}
}
}
@@ -935,13 +935,15 @@ void issueInSummary(TestInfo testInfo) throws IOException {

// The PR title does not match the issue title
assertTrue(pr.body().contains("Title mismatch"));
assertTrue(pr.body().contains("Integration blocker"));

// Correct it
pr.setTitle(issue2.id() + " - " + issue2.title());

// Check the status again - it should now match
TestBotRunner.runPeriodicItems(checkBot);
assertFalse(pr.body().contains("Title mismatch"));
assertFalse(pr.body().contains("Integration blocker"));

// Use an invalid issue key
var issueKey = issue1.id().replace("TEST", "BADPROJECT");
@@ -22,18 +22,15 @@
*/
package org.openjdk.skara.bots.pr;

import org.junit.jupiter.api.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.test.*;
import org.openjdk.skara.vcs.Repository;

import org.junit.jupiter.api.*;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.*;
import java.util.Set;
import java.util.stream.Collectors;

import static org.junit.jupiter.api.Assertions.*;
import static org.openjdk.skara.bots.pr.PullRequestAsserts.assertLastCommentContains;
@@ -235,11 +232,7 @@ void notReviewed(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
var error = pr.comments().stream()
.filter(comment -> comment.body().contains("integration request cannot be fulfilled at this time"))
.filter(comment -> comment.body().contains("failed the final jcheck"))
.count();
assertEquals(1, error, pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n---\n")));
assertLastCommentContains(pr, "PR has not yet been marked as ready for integration");
}
}

@@ -639,16 +632,20 @@ void cannotRebase(TestInfo testInfo) throws IOException {
var conflictingHash = CheckableRepository.appendAndCommit(localRepo, "This looks like a conflict");
localRepo.push(conflictingHash, author.url(), "master");

// Trigger a new check run
pr.setBody(pr.body() + " recheck");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
assertLastCommentContains(pr, "this pull request can not be integrated");

// Attempt an integration
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
var error = pr.comments().stream()
.filter(comment -> comment.body().contains("It was not possible to rebase your changes automatically."))
.filter(comment -> comment.body().contains("Please merge `master`"))
.count();
assertEquals(1, error);
TestBotRunner.runPeriodicItems(mergeBot);
assertLastCommentContains(pr, "PR has not yet been marked as ready for integration");
}
}

@@ -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,"Too few reviewers with at least role author found (have 0, need at least 1)");
assertLastCommentContains(reviewerPr,"PR has not yet been marked as ready for integration");

// Relax the requirement
reviewerPr.addComment("/reviewers 1");
@@ -239,7 +239,7 @@ void noSponsoring(TestInfo testInfo) throws IOException {
// It should not be possible to sponsor
reviewerPr.addComment("/sponsor");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(reviewerPr,"Too few reviewers with at least role author found (have 0, need at least 1)");
assertLastCommentContains(reviewerPr,"PR has not yet been marked as ready for integration");

// Relax the requirement
reviewerPr.addComment("/reviewers 1");
@@ -527,11 +527,7 @@ void sponsorAfterFailingCheck(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
var error = pr.comments().stream()
.filter(comment -> comment.body().contains("integration request cannot be fulfilled at this time"))
.filter(comment -> comment.body().contains("failed the final jcheck"))
.count();
assertEquals(1, error);
assertLastCommentContains(pr, "PR has not yet been marked as ready for integration");

// Make it ready for integration again
approvalPr.addReview(Review.Verdict.APPROVED, "Sorry, wrong button");
@@ -604,17 +600,20 @@ void cannotRebase(TestInfo testInfo) throws IOException {
var conflictingHash = CheckableRepository.appendAndCommit(localRepo, "This looks like a conflict");
localRepo.push(conflictingHash, author.url(), "master");

// Trigger a new check run
pr.setBody(pr.body() + " recheck");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
assertLastCommentContains(pr, "this pull request can not be integrated");

// Reviewer now agrees to sponsor
var reviewerPr = reviewer.pullRequest(pr.id());
reviewerPr.addComment("/sponsor");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
var error = pr.comments().stream()
.filter(comment -> comment.body().contains("It was not possible to rebase your changes automatically."))
.filter(comment -> comment.body().contains("Please merge `master`"))
.count();
assertEquals(1, error);
TestBotRunner.runPeriodicItems(mergeBot);
assertLastCommentContains(pr, "PR has not yet been marked as ready for integration");
}
}

1 comment on commit c963075

@bridgekeeper

This comment has been minimized.

Copy link

@bridgekeeper bridgekeeper bot commented on c963075 Sep 24, 2020

Please sign in to comment.
You can’t perform that action at this time.