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

1254: Add a checkbox for CSR requirement #1246

Closed
wants to merge 3 commits into from
Closed

1254: Add a checkbox for CSR requirement #1246

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80a0674
1254: Add a checkbox for CSR requirement
lgxbslgx Nov 27, 2021
9c02b6b
Merge branch 'master' into SKARA-1254-NEW
lgxbslgx Dec 1, 2021
fab5212
If the csr request is not required, it is no need to add the csr prog…
lgxbslgx Dec 1, 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
30 changes: 23 additions & 7 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
View file
Open in desktop
Expand Up @@ -167,6 +167,17 @@ 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")) {
ret.put("Change requires a CSR request to be approved", false);
} else {
ret.put("Change doesn't require a CSR request or the CSR request has been approved", true);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should show anything here if a CSR was never requested. The difference between "CSR was never requested" and "CSR is approved" isn't really tracked currently, so that would be tricky. I'm guessing this is a big reason why the original implementation didn't add checkboxes, but only a blocker message when active. For this to work we will need to add some state that tracks if a CSR is currently required. We basically have 3 states:

  1. No CSR needed
  2. CSR needed and not yet approved
  3. CSR needed and approved

Every PR starts in state 1. To get to state 2, you can either issue the /csr command, or with SKARA-1071 file a CSR and let Skara find it. To get to state 3, the CSR must be approved. At any time, the command "/csr unneeded" can take you back to state 1.

This could be implemented with another label, or it could rely on special comments with markers. In this case, I think we want to make it visible to users, so labels probably make more sense. For backwards compatibility, to keep the current meaning of the "csr" label, the new label would need to be something like "csr-approved" and be added at the same time the csr label is removed when moving to state 3.

Copy link
Member Author

Choose a reason for hiding this comment

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

@erikj79 The three states you mentioned above is clear. From your comment, I conclude the following things:

  1. No CSR needed: the PR doesn't have csr and csr-approved labels.
  2. CSR needed and not yet approved: the PR has csr label and doesn't have csr-approved label.
  3. CSR needed and approved: the PR has csr-approved label and doesn't have csr label.

And the csr label and csr-approved label can't appear at the same time.

But currently, the CSRBot and CSRCommand have the following logic:

  1. No CSR needed: the PR doesn't have csr label and the csr issue doesn't exist or has been withdrawn.
  2. CSR needed and not yet approved: the PR has csr label.
  3. CSR needed and approved: the PR doesn't have csr label and the csr issue has been Approved.

If we obey the new logic and create the new label csr-approved, some checks can be achieved easily. But the logic change may cause the unexpected regression. As you can see the discussion in the PR-1245 of the SKARA-1071, both the authors of the patch and the reviewers need to be very careful to avoid the misunderstanding and avoid making a mistake.

So I would like to obey the origin logic. In this patch, I can check the csr link and issue to identify the state 1 and state 3 which is like the CSRBot and CSRCommand. So we don't need to add another label such as csr-approved to solve this problem. What is your opinion about this?

And a related thing:
There are many places which need to get the csr issue from the original issue. We should provide a common method to solve this common problem. So I filed SKARA-1256 to solve it.

Copy link
Member

Choose a reason for hiding this comment

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

I needed to think about this for a bit, but I think you are right.

I was instinctively trying to keep the PR bot from having to query JBS for CSR status, because we currently have a label which made checking the CSR state very cheap and obvious. But on the other hand, we are fetching the issue from JBS anyway during every CheckRun, so adding this extra step isn't that expensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

@erikj79 I re-think about this. Even though we decide to use the new logic and add the new label csr-approved at the end, we shouldn't implement the feature at this patch. Because the goal of this patch is only to add a checkbox for CSR requirement.

So I advice to file a new issue to continue the discussion. The new issue title can be Investigating to add a label 'csr-approved' to mark the CSR is approved.

Back to this patch, I think it is good to use the old logic to implement the enhancement so that the users (OpenJDK developers) can take advantage of this patch as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a new label. Your suggestion to tell state 1 and 3 apart by checking if a CSR issue exists seems good enough to me.

}
return ret;
}

private void setExpiration(Duration expiresIn) {
// Use the shortest expiration
if (this.expiresIn == null || this.expiresIn.compareTo(expiresIn) > 0) {
Expand All @@ -175,8 +186,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() {
Expand Down Expand Up @@ -343,8 +353,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"));
Expand Down Expand Up @@ -426,12 +437,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()
Expand Down Expand Up @@ -898,6 +909,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
Expand All @@ -919,14 +931,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();

Expand All @@ -940,11 +954,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();
}

Expand Down
7 changes: 5 additions & 2 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/CSRTests.java
View file
Open in desktop
Expand Up @@ -531,6 +531,9 @@ void csrLabelShouldBlockReadyLabel(TestInfo testInfo) throws IOException {
var prAsAuthor = author.pullRequest(pr.id());
assertTrue(prAsAuthor.labelNames().contains("ready"));

// The body should contain the progress about CSR request
assertTrue(pr.body().contains("- [x] Change doesn't require a CSR request or the CSR request has been approved"));

// Require CSR
prAsReviewer = reviewer.pullRequest(pr.id());
prAsReviewer.addComment("/csr");
Expand All @@ -548,8 +551,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 body should contain the progress about CSR request
assertTrue(pr.body().contains("- [ ] Change requires a CSR request to be approved"));
}
}
}