Skip to content
Permalink
Browse files
1254: Add a checkbox for CSR requirement
Reviewed-by: ihse, erikj
  • Loading branch information
lgxbslgx authored and erikj79 committed Dec 1, 2021
1 parent eeb588f commit 08548eb733e3f37b558fee5ba6b3bd5380a9044c
Showing 2 changed files with 82 additions and 10 deletions.
@@ -198,6 +198,30 @@ private List<String> botSpecificChecks(boolean iscleanBackport) {
return ret;
}

// Additional bot-specific progresses that are not handled by JCheck
private Map<String, Boolean> botSpecificProgresses() {
var ret = new HashMap<String, Boolean>();
if (pr.labelNames().contains("csr")) {
// If the PR have csr label, the CSR request need to be approved.
ret.put("Change requires a CSR request to be approved", false);
} else {
var csrIssue = Issue.fromStringRelaxed(pr.title()).flatMap(this::getCsrIssue)
.flatMap(value -> issueProject() != null ? issueProject().issue(value.shortId()) : Optional.empty());
if (csrIssue.isPresent()) {
var resolution = csrIssue.get().properties().get("resolution");
if (resolution != null && !resolution.isNull()
&& resolution.get("name") != null && !resolution.get("name").isNull()
&& csrIssue.get().state() == org.openjdk.skara.issuetracker.Issue.State.CLOSED
&& "Approved".equals(resolution.get("name").asString())) {
// The PR doesn't have csr label and the csr request has been Approved.
ret.put("Change requires a CSR request to be approved", true);
}
}
// At other states, no need to add the csr progress.
}
return ret;
}

private void setExpiration(Duration expiresIn) {
// Use the shortest expiration
if (this.expiresIn == null || this.expiresIn.compareTo(expiresIn) > 0) {
@@ -206,8 +230,7 @@ private void setExpiration(Duration expiresIn) {
}

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.");
return Map.of("rejected", "The change is currently blocked from integration by a rejection.");
}

private List<String> botSpecificIntegrationBlockers() {
@@ -374,8 +397,9 @@ private String formatReviewer(HostUser reviewer) {
}
}

private String getChecksList(PullRequestCheckIssueVisitor visitor, boolean isCleanBackport) {
private String getChecksList(PullRequestCheckIssueVisitor visitor, boolean isCleanBackport, Map<String, Boolean> additionalProgresses) {
var checks = isCleanBackport ? visitor.getReadyForReviewChecks() : visitor.getChecks();
checks.putAll(additionalProgresses);
return checks.entrySet().stream()
.map(entry -> "- [" + (entry.getValue() ? "x" : " ") + "] " + entry.getKey())
.collect(Collectors.joining("\n"));
@@ -457,12 +481,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,
boolean isCleanBackport) {
List<String> additionalErrors, Map<String, Boolean> additionalProgresses,
List<String> integrationBlockers, boolean isCleanBackport) {
var progressBody = new StringBuilder();
progressBody.append("---------\n");
progressBody.append("### Progress\n");
progressBody.append(getChecksList(visitor, isCleanBackport));
progressBody.append(getChecksList(visitor, isCleanBackport, additionalProgresses));

var allAdditionalErrors = Stream.concat(visitor.hiddenMessages().stream(), additionalErrors.stream())
.sorted()
@@ -933,6 +957,7 @@ private void checkStatus() {
}

List<String> additionalErrors = List.of();
Map<String, Boolean> additionalProgresses = Map.of();
Hash localHash;
try {
// Do not pass eventual original commit even for backports since it will cause
@@ -954,14 +979,16 @@ private void checkStatus() {
var additionalConfiguration = AdditionalConfiguration.get(localRepo, localHash, pr.repository().forge().currentUser(), comments);
checkablePullRequest.executeChecks(localHash, censusInstance, visitor, additionalConfiguration);
additionalErrors = botSpecificChecks(isCleanBackport);
additionalProgresses = botSpecificProgresses();
}
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, integrationBlockers, isCleanBackport);
var statusMessage = getStatusMessage(comments, activeReviews, visitor, additionalErrors,
additionalProgresses, integrationBlockers, isCleanBackport);
var updatedBody = updateStatusMessage(statusMessage);
var title = pr.title();

@@ -975,11 +1002,13 @@ private void checkStatus() {
var commitMessage = String.join("\n", commit.message());
var readyForIntegration = visitor.messages().isEmpty() &&
additionalErrors.isEmpty() &&
!additionalProgresses.containsValue(false) &&
integrationBlockers.isEmpty();
if (isCleanBackport) {
// Reviews are not needed for clean backports
readyForIntegration = visitor.isReadyForReview() &&
additionalErrors.isEmpty() &&
!additionalProgresses.containsValue(false) &&
integrationBlockers.isEmpty();
}

@@ -72,7 +72,8 @@ void simple(TestInfo testInfo) throws IOException {
"[compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request " +
"is needed for this pull request.");
assertTrue(pr.labelNames().contains("csr"));

// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// No longer require CSR
prAsReviewer.addComment("/csr unneeded");
@@ -82,6 +83,8 @@ void simple(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "determined that a [CSR](https://wiki.openjdk.java.net/display/csr/Main) request " +
"is not needed for this pull request.");
assertFalse(pr.labelNames().contains("csr"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));

// Require CSR again with long form
prAsReviewer.addComment("/csr needed");
@@ -92,6 +95,8 @@ void simple(TestInfo testInfo) throws IOException {
"[compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request " +
"is needed for this pull request.");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}

@@ -136,6 +141,8 @@ void alreadyApprovedCSR(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "the issue for this pull request");
assertLastCommentContains(pr, "already has an approved CSR request");
assertFalse(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [x] Change requires a CSR request to be approved"));
}
}

@@ -176,6 +183,8 @@ void testMissingIssue(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "this pull request must refer to an issue in [JBS]");
assertLastCommentContains(pr, "To refer this pull request to an issue in JBS");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}

@@ -213,13 +222,17 @@ void requireCSRAsCommitter(TestInfo testInfo) throws IOException {
assertLastCommentContains(prAsAnother, "only the pull request author and [Reviewers]");
assertLastCommentContains(prAsAnother, "are allowed to use the `csr` command.");
assertFalse(prAsAnother.labelNames().contains("csr"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));

// Stating that a CSR is not needed should not work
prAsAnother.addComment("/csr unneeded");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(prAsAnother, "only the pull request author and [Reviewers]");
assertLastCommentContains(prAsAnother, "are allowed to use the `csr` command.");
assertFalse(prAsAnother.labelNames().contains("csr"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));

// Require CSR as committer
pr.addComment("/csr");
@@ -230,20 +243,26 @@ void requireCSRAsCommitter(TestInfo testInfo) throws IOException {
"[compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request " +
"is needed for this pull request.");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// Stating that a CSR is not needed should not work
pr.addComment("/csr unneeded");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "only [Reviewers]");
assertLastCommentContains(pr, "can determine that a CSR is not needed.");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// Stating that a CSR is not needed should not work
prAsAnother.addComment("/csr unneeded");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(prAsAnother, "only the pull request author and [Reviewers]");
assertLastCommentContains(prAsAnother, "are allowed to use the `csr` command.");
assertTrue(prAsAnother.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}

@@ -281,6 +300,8 @@ void showHelpMessageOnUnexpectedArg(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "usage: `/csr [needed|unneeded]`, requires that the issue the pull request refers to links " +
"to an approved [CSR](https://wiki.openjdk.java.net/display/csr/Main) request.");
assertFalse(pr.labelNames().contains("csr"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));
}
}

@@ -322,6 +343,8 @@ void nonExistingJBSIssue(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "this pull request must refer to an issue in [JBS]");
assertLastCommentContains(pr, "to be able to link it to a [CSR](https://wiki.openjdk.java.net/display/csr/Main) request. To refer this pull request to an issue in JBS");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}

@@ -362,6 +385,8 @@ void csrRequestWhenCSRIsAlreadyRequested(TestInfo testInfo) throws IOException {
"[compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request " +
"is needed for this pull request.");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// Require a CSR again
prAsReviewer.addComment("/csr");
@@ -371,6 +396,8 @@ void csrRequestWhenCSRIsAlreadyRequested(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "an approved [CSR]");
assertLastCommentContains(pr, "request is already required for this pull request.");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}

@@ -415,6 +442,8 @@ void notYetApprovedCSR(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "this pull request will not be integrated until the [CSR]");
assertLastCommentContains(pr, "for issue ");
assertLastCommentContains(pr, "has been approved.");
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// Indicate the PR doesn't require CSR, but it doesn't work.
prAsReviewer.addComment("/csr unneeded");
@@ -427,6 +456,8 @@ void notYetApprovedCSR(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "Please firstly withdraw the CSR request");
assertLastCommentContains(pr, "and then use the command `/csr unneeded` again");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// withdraw the csr
csr.setState(Issue.State.CLOSED);
@@ -440,6 +471,8 @@ void notYetApprovedCSR(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "determined that a [CSR](https://wiki.openjdk.java.net/display/csr/Main) request " +
"is not needed for this pull request.");
assertFalse(pr.labelNames().contains("csr"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));
}
}

@@ -483,6 +516,8 @@ void csrWithNullResolution(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "this pull request will not be integrated until the [CSR]");
assertLastCommentContains(pr, "for issue ");
assertLastCommentContains(pr, "has been approved.");
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// Indicate the PR doesn't require CSR, but it doesn't work.
prAsReviewer.addComment("/csr unneeded");
@@ -495,6 +530,8 @@ void csrWithNullResolution(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "Please firstly withdraw the CSR request");
assertLastCommentContains(pr, "and then use the command `/csr unneeded` again");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));

// withdraw the csr
csr.setState(Issue.State.CLOSED);
@@ -508,6 +545,8 @@ void csrWithNullResolution(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "determined that a [CSR](https://wiki.openjdk.java.net/display/csr/Main) request " +
"is not needed for this pull request.");
assertFalse(pr.labelNames().contains("csr"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));
}
}

@@ -542,6 +581,8 @@ void csrInPrBody(TestInfo testInfo) throws IOException {
"[compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request " +
"is needed for this pull request.");
assertTrue(pr.labelNames().contains("csr"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}

@@ -580,6 +621,8 @@ void csrLabelShouldBlockReadyLabel(TestInfo testInfo) throws IOException {
// PR should be ready
var prAsAuthor = author.pullRequest(pr.id());
assertTrue(prAsAuthor.labelNames().contains("ready"));
// The PR body shouldn't contain the progress about CSR request
assertFalse(pr.body().contains("Change requires a CSR request to be approved"));

// Require CSR
prAsReviewer = reviewer.pullRequest(pr.id());
@@ -598,8 +641,8 @@ void csrLabelShouldBlockReadyLabel(TestInfo testInfo) throws IOException {
prAsAuthor = author.pullRequest(pr.id());
assertFalse(prAsAuthor.labelNames().contains("ready"));

// The body should contain a note about why
assertTrue(pr.body().contains("change requires a CSR request to be approved"));
// The PR body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}
}

1 comment on commit 08548eb

@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented on 08548eb Dec 1, 2021

Choose a reason for hiding this comment

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

Please sign in to comment.