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

Jump to bottom

1026: Skara bot doesn't update Progress checklist for clean backports to show that it is properly reviewed #1198

Closed
wants to merge 3 commits into from
+28 −8
Closed

1026: Skara bot doesn't update Progress checklist for clean backports to show that it is properly reviewed #1198

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5afa58a
SKARA-1026
erikj79 Jul 14, 2021
24cfb88
Add check in test for non clean backport
erikj79 Jul 14, 2021
9c266f6
Add comments
erikj79 Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
View file
Open in desktop
@@ -340,10 +340,11 @@ private String formatReviewer(HostUser reviewer) {
}
}

private String getChecksList(PullRequestCheckIssueVisitor visitor) {
return visitor.getChecks().entrySet().stream()
.map(entry -> "- [" + (entry.getValue() ? "x" : " ") + "] " + entry.getKey())
.collect(Collectors.joining("\n"));
private String getChecksList(PullRequestCheckIssueVisitor visitor, boolean isCleanBackport) {
var checks = isCleanBackport ? visitor.getReadyForReviewChecks() : visitor.getChecks();
Copy link
Member

@kevinrushforth kevinrushforth Jul 14, 2021

Choose a reason for hiding this comment

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

I find the name of the new method a little confusing. A clean backport is ready for integration not for review. Maybe just getReadyChecks or getChecksClean? I'll leave it up to you.

Copy link
Member Author

@erikj79 erikj79 Jul 14, 2021

Choose a reason for hiding this comment

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

I understand the confusion. I chose the name to fit in with the current model and naming. When CheckRun evaluates if a cleanBackport PR is ready for integration, it calls PullRequestCheckIssueVisitor.isReadyForReview() (instead of .messages().isEmpty()). So the state we are checking for in the visitor is already named "readyForReview". The new method builds on that and returns the checks that were used to decide if it was readyForReview.

Copy link
Member Author

@erikj79 erikj79 Jul 14, 2021

Choose a reason for hiding this comment

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

I should add that the CheckRun knows about clean backports, while the visitor does not, it just evaluates Jcheck checks and reports on the outcome.

Copy link
Member

@kevinrushforth kevinrushforth Jul 14, 2021

Choose a reason for hiding this comment

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

OK, thanks for the explanation.

return checks.entrySet().stream()
.map(entry -> "- [" + (entry.getValue() ? "x" : " ") + "] " + entry.getKey())
.collect(Collectors.joining("\n"));
}

private String warningListToText(List<String> additionalErrors) {
@@ -417,11 +418,12 @@ private boolean relaxedEquals(String s1, String s2) {
}

private String getStatusMessage(List<Comment> comments, List<Review> reviews, PullRequestCheckIssueVisitor visitor,
List<String> additionalErrors, List<String> integrationBlockers) {
List<String> additionalErrors, List<String> integrationBlockers,
boolean isCleanBackport) {
var progressBody = new StringBuilder();
progressBody.append("---------\n");
progressBody.append("### Progress\n");
progressBody.append(getChecksList(visitor));
progressBody.append(getChecksList(visitor, isCleanBackport));

var allAdditionalErrors = Stream.concat(visitor.hiddenMessages().stream(), additionalErrors.stream())
.sorted()
@@ -917,7 +919,7 @@ private void checkStatus() {
var integrationBlockers = botSpecificIntegrationBlockers();

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

14 bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java
View file
Open in desktop
@@ -68,13 +68,27 @@ List<String> hiddenMessages() {
.collect(Collectors.toList());
}

/**
* Get all the displayed checks with results.
*/
Map<String, Boolean> getChecks() {
return enabledChecks.stream()
.filter(check -> displayedChecks.contains(check.getClass()))
.collect(Collectors.toMap(Check::description,
check -> !failedChecks.containsKey(check.getClass())));
}

/**
* Get all the displayed checks with results that were used to decide if this change is ready for
* review.
*/
Map<String, Boolean> getReadyForReviewChecks() {
return enabledChecks.stream()
.filter(check -> displayedChecks.contains(check.getClass()))
.filter(check -> !(check instanceof ReviewersCheck))
.collect(Collectors.toMap(Check::description,
check -> !failedChecks.containsKey(check.getClass())));
}

List<CheckAnnotation> getAnnotations() { return annotations; }

3 bots/pr/src/test/java/org/openjdk/skara/bots/pr/BackportTests.java
View file
Open in desktop
@@ -24,6 +24,7 @@

import org.junit.jupiter.api.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.jcheck.ReviewersCheck;
import org.openjdk.skara.test.*;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.CommitMessageParsers;
@@ -577,6 +578,7 @@ void cleanBackport(TestInfo testInfo) throws IOException {
assertTrue(backportComment.contains("<!-- backport " + releaseHash.hex() + " -->"));
assertEquals(issue1Number + ": An issue", pr.title());
assertTrue(pr.labelNames().contains("backport"));
assertFalse(pr.body().contains(ReviewersCheck.DESCRIPTION), "Reviewer requirement found in pr body");

// The bot should have added the "clean" label
assertTrue(pr.labelNames().contains("clean"));
@@ -717,6 +719,7 @@ void notCleanBackport(TestInfo testInfo) throws IOException {
assertTrue(backportComment.contains("<!-- backport " + upstreamHash.hex() + " -->"));
assertEquals(issue2Number + ": Another issue", pr.title());
assertTrue(pr.labelNames().contains("backport"));
assertTrue(pr.body().contains(ReviewersCheck.DESCRIPTION), "Reviewer requirement not found in pr body");

// The bot should not have added the "clean" label
assertFalse(pr.labelNames().contains("clean"));
3 jcheck/src/main/java/org/openjdk/skara/jcheck/ReviewersCheck.java
View file
Open in desktop
@@ -36,6 +36,7 @@
import java.util.logging.Logger;

public class ReviewersCheck extends CommitCheck {
public static final String DESCRIPTION = "Change must be properly reviewed";
private final Logger log = Logger.getLogger("org.openjdk.skara.jcheck.reviewers");
private final Utilities utils;

@@ -169,6 +170,6 @@ public String name() {

@Override
public String description() {
return "Change must be properly reviewed";
return DESCRIPTION;
}
}