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.

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

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

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -170,9 +170,6 @@ private IssueProject issueProject() {
ret.add(error);
}

var headHash = pr.headHash();
var originalCommits = localRepo.commitMetadata(baseHash, headHash);

for (var blocker : workItem.bot.blockingCheckLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
ret.add(blocker.getValue());
@@ -182,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);
@@ -266,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)
@@ -337,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");
@@ -352,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();
@@ -724,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();

@@ -739,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");
@@ -523,11 +523,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");
@@ -600,17 +596,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");
}
}

ProTip! Use n and p to navigate between commits in a pull request.