Skip to content
Permalink
Browse files
599: Unverified reviews should not count towards jcheck 
Reviewed-by: ehelin
  • Loading branch information
@rwestberg
rwestberg committed Sep 9, 2020
1 parent ea4920d commit d23a796d3e5de8cb5b4f86d9d946cfa0888726a8
Showing 6 changed files with 70 additions and 54 deletions.
3 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
View file
@@ -758,7 +758,8 @@ private void checkStatus() {
updateReviewedMessages(comments, allReviews);
}

var commit = localRepo.lookup(localHash).orElseThrow();
var amendedHash = checkablePullRequest.amendManualReviewers(localHash, censusInstance.namespace());
var commit = localRepo.lookup(amendedHash).orElseThrow();
var commitMessage = String.join("\n", commit.message());
var readyForIntegration = visitor.messages().isEmpty() && additionalErrors.isEmpty();
updateMergeReadyComment(readyForIntegration, commitMessage, comments, activeReviews, rebasePossible);
18 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java
View file
@@ -53,7 +53,7 @@ public class CheckablePullRequest {
}
}

private String commitMessage(List<Review> activeReviews, Namespace namespace) throws IOException {
private String commitMessage(List<Review> activeReviews, Namespace namespace, boolean manualReviewers) throws IOException {
var eligibleReviews = activeReviews.stream()
.filter(review -> !ignoreStaleReviews || review.hash().equals(pr.headHash()))
.filter(review -> review.verdict() == Review.Verdict.APPROVED)
@@ -62,7 +62,7 @@ private String commitMessage(List<Review> activeReviews, Namespace namespace) th
var comments = pr.comments();
var currentUser = pr.repository().forge().currentUser();

if (!ignoreStaleReviews) {
if (manualReviewers) {
var allReviewers = PullRequestUtils.reviewerNames(activeReviews, namespace);
var additionalReviewers = Reviewers.reviewers(currentUser, comments);
for (var additionalReviewer : additionalReviewers) {
@@ -130,10 +130,22 @@ Hash commit(Hash finalHead, Namespace namespace, String censusDomain, String spo
}

var activeReviews = filterActiveReviews(pr.reviews());
var commitMessage = commitMessage(activeReviews, namespace);
var commitMessage = commitMessage(activeReviews, namespace, false);
return PullRequestUtils.createCommit(pr, localRepo, finalHead, author, committer, commitMessage);
}

Hash amendManualReviewers(Hash commit, Namespace namespace) throws IOException {
var activeReviews = filterActiveReviews(pr.reviews());
var originalCommitMessage = commitMessage(activeReviews, namespace, false);
var amendedCommitMessage = commitMessage(activeReviews, namespace, true);

if (originalCommitMessage.equals(amendedCommitMessage)) {
return commit;
} else {
return localRepo.amend(amendedCommitMessage);
}
}

PullRequestCheckIssueVisitor createVisitor(Hash localHash, CensusInstance censusInstance) throws IOException {
var checks = JCheck.checksFor(localRepo, censusInstance.census(), pr.targetHash());
return new PullRequestCheckIssueVisitor(checks);
5 bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java
View file
@@ -151,14 +151,15 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

// Rebase and push it!
if (!localHash.equals(pr.targetHash())) {
var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace());
var finalRebaseMessage = rebaseMessage.toString();
if (!finalRebaseMessage.isBlank()) {
reply.println(rebaseMessage.toString());
}
reply.println("Pushed as commit " + localHash.hex() + ".");
reply.println("Pushed as commit " + amendedHash.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.");
localRepo.push(localHash, pr.repository().url(), pr.targetRef());
localRepo.push(amendedHash, pr.repository().url(), pr.targetRef());
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
pr.removeLabel("ready");
26 bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java
View file
@@ -32,14 +32,14 @@
import java.util.regex.Pattern;

public class ReviewerCommand implements CommandHandler {
private static final Pattern commandPattern = Pattern.compile("^(add|remove)\\s+(.+)$");
private static final Pattern commandPattern = Pattern.compile("^(credit|remove)\\s+(.+)$");

private void showHelp(PullRequest pr, PrintWriter reply) {
reply.println("Syntax: `/reviewer (add|remove) [@user | openjdk-user]+`. For example:");
reply.println("Syntax: `/reviewer (credit|remove) [@user | openjdk-user]+`. For example:");
reply.println();
reply.println(" * `/reviewer add @openjdk-bot`");
reply.println(" * `/reviewer add duke`");
reply.println(" * `/reviewer add @user1 @user2`");
reply.println(" * `/reviewer credit @openjdk-bot`");
reply.println(" * `/reviewer credit duke`");
reply.println(" * `/reviewer credit @user1 @user2`");
}

private Optional<Contributor> parseUser(String user, PullRequest pr, CensusInstance censusInstance) {
@@ -72,10 +72,6 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `reviewer` command.");
return;
}
if (bot.ignoreStaleReviews()) {
reply.println("This project requires authenticated reviews - please ask your reviewer to flag this PR as reviewed.");
return;
}

var matcher = commandPattern.matcher(command.args());
if (!matcher.matches()) {
@@ -98,13 +94,13 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
var namespace = censusInstance.namespace();
var authenticatedReviewers = PullRequestUtils.reviewerNames(pr.reviews(), namespace);
var action = matcher.group(1);
if (action.equals("add")) {
if (action.equals("credit")) {
for (var reviewer : reviewers) {
if (!authenticatedReviewers.contains(reviewer.username())) {
reply.println(Reviewers.addReviewerMarker(reviewer));
reply.println("Reviewer `" + reviewer.username() + "` successfully added.");
reply.println("Reviewer `" + reviewer.username() + "` successfully credited.");
} else {
reply.println("Reviewer `" + reviewer.username() + "` has already made an authenticated review of this PR, and does not need to be added manually.");
reply.println("Reviewer `" + reviewer.username() + "` has already made an authenticated review of this PR, and does not need to be credited manually.");
}
}
} else if (action.equals("remove")) {
@@ -116,7 +112,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
reply.println("Reviewer `" + reviewer.username() + "` successfully removed.");
} else {
if (existing.isEmpty()) {
reply.println("There are no additional reviewers associated with this pull request.");
reply.println("There are no manually specified reviewers associated with this pull request.");
failed = true;
} else {
reply.println("Reviewer `" + reviewer.username() + "` was not found.");
@@ -126,7 +122,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

if (failed) {
reply.println("Current additional reviewers are:");
reply.println("Current credited reviewers are:");
for (var e : existing) {
reply.println("- `" + e + "`");
}
@@ -136,6 +132,6 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

@Override
public String description() {
return "adds or removes additional reviewers for a PR";
return "manage additional reviewers for a PR";
}
}
5 bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java
View file
@@ -125,14 +125,15 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

if (!localHash.equals(pr.targetHash())) {
var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace());
var finalRebaseMessage = rebaseMessage.toString();
if (!finalRebaseMessage.isBlank()) {
reply.println(rebaseMessage.toString());
}
reply.println("Pushed as commit " + localHash.hex() + ".");
reply.println("Pushed as commit " + amendedHash.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.");
localRepo.push(localHash, pr.repository().url(), pr.targetRef());
localRepo.push(amendedHash, pr.repository().url(), pr.targetRef());
pr.setState(PullRequest.State.CLOSED);
pr.addLabel("integrated");
pr.removeLabel("sponsor");
67 bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewerTests.java
View file
@@ -68,11 +68,11 @@ void simple(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr,"Syntax");

// Add a reviewer
pr.addComment("/reviewer add @" + integrator.forge().currentUser().userName());
pr.addComment("/reviewer credit @" + integrator.forge().currentUser().userName());
TestBotRunner.runPeriodicItems(prBot);

// The bot should now consider the PR ready
assertLastCommentContains(pr,"This change now passes all automated pre-integration checks");
// The bot should not yet consider the PR ready
assertFalse(pr.labels().contains("ready"));

// Remove it again
pr.addComment("/reviewer remove @" + integrator.forge().currentUser().userName());
@@ -86,10 +86,10 @@ void simple(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(prBot);

// The bot should reply with an error message
assertLastCommentContains(pr,"There are no additional reviewers associated with this pull request");
assertLastCommentContains(pr,"There are no manually specified reviewers associated with this pull request");

// Add the reviewer again
pr.addComment("/reviewer add integrationreviewer1");
pr.addComment("/reviewer credit integrationreviewer1");
TestBotRunner.runPeriodicItems(prBot);

// But also add the review the old-fashioned way
@@ -112,7 +112,7 @@ void simple(TestInfo testInfo) throws IOException {
assertEquals(1, pushed);

// Add a second reviewer
pr.addComment("/reviewer add integrationauthor2");
pr.addComment("/reviewer credit integrationauthor2");
TestBotRunner.runPeriodicItems(prBot);
TestBotRunner.runPeriodicItems(prBot);

@@ -172,7 +172,7 @@ void invalidCommandAuthor(TestInfo testInfo) throws IOException {

// Issue a contributor command not as the PR author
var externalPr = external.pullRequest(pr.id());
externalPr.addComment("/reviewer add integrationauthor1");
externalPr.addComment("/reviewer credit integrationauthor1");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
@@ -208,22 +208,22 @@ void invalidReviewer(TestInfo testInfo) throws IOException {
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Use a full name
pr.addComment("/reviewer add Moo <Foo.Bar (at) host.com>");
pr.addComment("/reviewer credit Moo <Foo.Bar (at) host.com>");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "Could not parse `Moo` as a valid reviewer");

// Empty platform id
pr.addComment("/reviewer add @");
pr.addComment("/reviewer credit @");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "Could not parse `@` as a valid reviewer");

// Unknown platform id
pr.addComment("/reviewer add @someone");
pr.addComment("/reviewer credit @someone");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "Could not parse `@someone` as a valid reviewer");

// Unknown openjdk user
pr.addComment("/reviewer add someone");
pr.addComment("/reviewer credit someone");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "Could not parse `someone` as a valid reviewer");
}
@@ -254,11 +254,11 @@ void platformUser(TestInfo testInfo) throws IOException {
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Use a platform name
pr.addComment("/reviewer add @" + author.forge().currentUser().userName());
pr.addComment("/reviewer credit @" + author.forge().currentUser().userName());
TestBotRunner.runPeriodicItems(prBot);

// The bot should reply
assertLastCommentContains(pr, "Reviewer `integrationcommitter2` successfully added.");
assertLastCommentContains(pr, "Reviewer `integrationcommitter2` successfully credited.");
}
}

@@ -287,11 +287,11 @@ void openJdkUser(TestInfo testInfo) throws IOException {
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Use a platform name
pr.addComment("/reviewer add integrationauthor1");
pr.addComment("/reviewer credit integrationauthor1");
TestBotRunner.runPeriodicItems(prBot);

// The bot should reply
assertLastCommentContains(pr, "Reviewer `integrationauthor1` successfully added.");
assertLastCommentContains(pr, "Reviewer `integrationauthor1` successfully credited.");
}
}

@@ -322,18 +322,18 @@ void removeReviewer(TestInfo testInfo) throws IOException {
// Remove a reviewer that hasn't been added
pr.addComment("/reviewer remove integrationauthor1");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "There are no additional reviewers associated with this pull request.");
assertLastCommentContains(pr, "There are no manually specified reviewers associated with this pull request.");

// Add a reviewer
pr.addComment("/reviewer add integrationauthor1");
pr.addComment("/reviewer credit integrationauthor1");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "successfully added.");
assertLastCommentContains(pr, "successfully credited.");

// Remove another (not added) reviewer
pr.addComment("/reviewer remove integrationcommitter2");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "Reviewer `integrationcommitter2` was not found.");
assertLastCommentContains(pr, "Current additional reviewers are:");
assertLastCommentContains(pr, "Current credited reviewers are:");
assertLastCommentContains(pr, "- `integrationauthor1`");

// Remove an existing reviewer
@@ -368,10 +368,10 @@ void prBodyUpdates(TestInfo testInfo) throws IOException {
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Add a reviewer
pr.addComment("/reviewer add integrationauthor1");
pr.addComment("/reviewer credit integrationauthor1");
TestBotRunner.runPeriodicItems(prBot);
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "successfully added.");
assertLastCommentContains(pr, "successfully credited.");

// Verify that body is updated
var body = pr.body().split("\n");
@@ -403,10 +403,10 @@ void prBodyUpdates(TestInfo testInfo) throws IOException {
assertFalse(pr.body().contains("Added manually"));

// Add it once more
pr.addComment("/reviewer add integrationauthor1");
pr.addComment("/reviewer credit integrationauthor1");
TestBotRunner.runPeriodicItems(prBot);
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "successfully added.");
assertLastCommentContains(pr, "successfully credited.");
assertTrue(pr.body().contains("Added manually"));

// Now add an authenticated review from the same reviewer
@@ -452,7 +452,7 @@ void addAuthenticated(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(prBot);

// Try to add it manually as well
pr.addComment("/reviewer add integrationreviewer1");
pr.addComment("/reviewer credit integrationreviewer1");
TestBotRunner.runPeriodicItems(prBot);
TestBotRunner.runPeriodicItems(prBot);

@@ -471,7 +471,7 @@ void multiple(TestInfo testInfo) throws IOException {

var censusBuilder = credentials.getCensusBuilder()
.addReviewer(integrator.forge().currentUser().id())
.addAuthor(extra.forge().currentUser().id())
.addReviewer(extra.forge().currentUser().id())
.addCommitter(author.forge().currentUser().id());
var prBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build();

@@ -487,20 +487,25 @@ void multiple(TestInfo testInfo) throws IOException {
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Add two reviewers
pr.addComment("/reviewer add integrationreviewer1 integrationauthor2");
// Credit two additional reviewers
pr.addComment("/reviewer credit integrationreviewer1 integrationcommitter3");
TestBotRunner.runPeriodicItems(prBot);

// Expect success
assertLastCommentContains(pr, "Reviewed-by: integrationreviewer1, integrationauthor2");
// Add a real review
var approvalPr = extra.pullRequest(pr.id());
approvalPr.addReview(Review.Verdict.APPROVED, "Approved");
TestBotRunner.runPeriodicItems(prBot);

// Check the ready comment
assertLastCommentContains(pr, "Reviewed-by: integrationreviewer2, integrationreviewer1, integrationcommitter3");

// Remove both reviewers
pr.addComment("/reviewer remove integrationreviewer1 integrationauthor2");
pr.addComment("/reviewer remove integrationreviewer1 integrationcommitter3");
TestBotRunner.runPeriodicItems(prBot);

// Expect success
assertLastCommentContains(pr, "Reviewer `integrationreviewer1` successfully removed");
assertLastCommentContains(pr, "Reviewer `integrationauthor2` successfully removed");
assertLastCommentContains(pr, "Reviewer `integrationcommitter3` successfully removed");
}
}
}

1 comment on commit d23a796

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented on d23a796 Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.