RFR: 1714: CSR detection only happens on the title bug of a PR
Erik Joelsson
erikj at openjdk.org
Thu Dec 15 17:53:49 UTC 2022
On Wed, 14 Dec 2022 01:08:08 GMT, Zhao Song <zsong at openjdk.org> wrote:
> Currently, our bots only detect CSR issues that are directly linked to the main issue.
>
> With this patch, our bots will be able to detect CSR issues that are linked to all of the issues that this PR solves. Additionally, all detected CSR issues will affect the integration blockers.
>
> The `/csr needed` command will allow you to create a CSR issue that is linked to any of the issues solved by this PR. The `/csr unneeded` command will be blocked if any of the issues associated with this PR have an active CSR issue.
I've made a first pass over this patch. It's a rather complicated change which introduces a lot of corner cases that need careful reasoning and testing. I found some issues that I've pointed out, but there could definitely be more. We also know that the current CSR implementation has issues, which may get amplified by this change. I'm still not sure if we should risk going ahead with this before we figure out a better solution to the whole CSR functionality.
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 96:
> 94: var statusMessage = getStatusMessage(pr);
> 95: return hasCsrIssue(statusMessage, csr) &&
> 96: (statusMessage.contains("- [ ] Change requires a CSR request (" + csr.id() + ") to be approved") ||
Please remove the 'a' and parentheses when the CSR issue number is known.
Suggestion:
(statusMessage.contains("- [ ] Change requires CSR request " + csr.id() + " to be approved") ||
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 136:
> 134: var issues = new ArrayList<org.openjdk.skara.vcs.openjdk.Issue>();
> 135: mainIssue.ifPresent(issues::add);
> 136: issues.addAll(SolvesTracker.currentSolved(pr.repository().forge().currentUser(), pr.comments()));
The `SolvesTracker` in the pr bot shouldn't be reachable from this module. If you need it in multiple bots, then please move it to bots.utils.
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 154:
> 152: var jbsIssueOpt = project.issue(issue.shortId());
> 153: if (jbsIssueOpt.isEmpty()) {
> 154: // One issue could not be found in JBS, so the csr label cannot be removed until the problem is solved
Suggestion:
// An issue could not be found, so the csr label cannot be removed
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 157:
> 155: allCSRApproved = false;
> 156: var issueId = issue.project().isEmpty() ? (project.name() + "-" + issue.id()) : issue.id();
> 157: log.info("No issue found in JBS related with this issue " + issueId + "for " + describe(pr));
Suggestion:
log.info("issueId + " for " + describe(pr) + " not found");
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 158:
> 156: var issueId = issue.project().isEmpty() ? (project.name() + "-" + issue.id()) : issue.id();
> 157: log.info("No issue found in JBS related with this issue " + issueId + "for " + describe(pr));
> 158: // allCSRApproved is false now, so we could break now
Suggestion:
// allCSRApproved is now false, so there is no point in continuing
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 165:
> 163: if (csrOptional.isEmpty()) {
> 164: log.info("No CSR found for issue " + jbsIssueOpt.get().id() + " for " + describe(pr));
> 165: continue;
What happens if no issue has a CSR but the label was added using the CSR command? In that case we should not remove the label.
bots/csr/src/main/java/org/openjdk/skara/bots/csr/PullRequestWorkItem.java line 175:
> 173: log.info("The PR body doesn't have the CSR issue or progress, adding the csr update marker for this csr issue"
> 174: + csr.id() + " for " + describe(pr));
> 175: addUpdateMarker(pr);
`addUpdateMarker` isn't suitable to be called multiple times. Each call will modify the body again.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 61:
> 59: writer.println("@" + pr.author().username() + " please create a [CSR](https://wiki.openjdk.org/display/csr/Main) " +
> 60: "request for any issue associated with this pr with the correct fix version " +
> 61: "This pull request cannot be integrated until the CSR request is approved.");
This message doesn't really make sense. If there is more than one issue associated with the pr, then I don't think there is any point printing this at all. We can't know which issue that needs a CSR, so the person issuing the command has to be responsible for clarifying that. The main point of this message is to link to the issue.
So, if there is only one issue (which will be the case in 99% of cases involving CSRs), print the old message, otherwise don't call this method, or have it do nothing.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 136:
> 134: // The issue has a non-withdrawn csr issue, the bot should direct the user to withdraw the csr firstly.
> 135: reply.println("The CSR requirement cannot be removed as there is already a CSR associated with the issue [" +
> 136: jbsIssue.id() + "](" + jbsIssue.webUrl() + ") of this pull request. Please withdraw the CSR ["
Suggestion:
jbsIssue.id() + "](" + jbsIssue.webUrl() + "). Please withdraw the CSR ["
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 142:
> 140: }
> 141: }
> 142: // All the issue associated with this pr don't have csr issue or the csr issue has already been withdrawn,
Suggestion:
// All the issues associated with this pr either don't have csr issue or the csr issue has been withdrawn,
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 193:
> 191: continue;
> 192: }
> 193: // Could find a csr issue for one of the issues associated with this pr
Suggestion:
// Found a csr issue for one of the issues associated with this pr
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 217:
> 215: pr.addLabel(CSR_LABEL);
> 216: }
> 217: return;
I'm not sure about this behavior. We loop until we find an existing CSR, then print a reply based on the state of that CSR.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 219:
> 217: return;
> 218: }
> 219: // All the issues associated with pr don't have csr issue or the csr issue has already been withdrawn
Suggestion:
// All the issues associated with pr either don't have csr issue or the csr issue has already been withdrawn
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 268:
> 266: .toList();
> 267: if (csrIssues.isEmpty() && pr.labelNames().contains("csr")) {
> 268: ret.put("Change requires a CSR request (need to be created) to be approved", false);
Suggestion:
ret.put("Change requires a CSR request (needs to be created) to be approved", false);
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 272:
> 270: for (var csrIssue : csrIssues) {
> 271: if (!csrIssue.isClosed()) {
> 272: ret.put("Change requires a CSR request (" + csrIssue.id() + ") to be approved", false);
I think we can make the issue IDs links here.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 661:
> 659: if (issueType != null && "CSR".equals(issueType.asString())) {
> 660: progressBody.append(" (**CSR**)");
> 661: if(isWithdrawnCSR(iss.get())){
Suggestion:
if (isWithdrawnCSR(iss.get())) {
-------------
PR: https://git.openjdk.org/skara/pull/1443
More information about the skara-dev
mailing list