RFR: 1850: CSR bot can pick up wrong fix version if PR branch is behind target branch [v2]

[Erik Joelsson]

On Wed, 5 Apr 2023 18:02:19 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> As SKARA-1851 fixed, now we could be able to search for CSR with the  fixVersion in .jcheck/conf from the "merge branch".
>> 
>> There are also some changes to CSR Command in this patch.
>> 
>> Originally, we need to search for CSR issues in CSR Command because updating CSR progress in PR body needs cooperation between CSR bot and PR bot(so we couldn’t trust the CSR progress in PR body).
>> 
>> But now, we could force the update of PR body before handling the CSR command.
>> 
>> The logic of handling CSR command is also easy now.
>> 
>> For `/csr unneeded`:
>> 
>> Case 1: There exists CSR progress in the pr body, in this case, the csr requirement cannot be removed and we prompt user that 'withdraw csr first then try again'
>> Case 2: No CSR progress in the pr body, it means all the issues this pr solves don't have csr issue or the csr issue has already been withdrawn. In this case, we could remove the csr requirement safely.
>> 
>> For '/csr needed':
>> 
>> Case 1: The pr has csr label. We just tell the user csr is already require for this pr.
>> 
>> Case 2: The pr doesn't have csr label.
>> In this case, there are only three possibilities
>> (i) CSR is never requested and No CSR issues been created. In this case, we add CSR label to this pr and give user some prompts.
>> (ii) ALL CSR issues are withdrawn. In this case, we do the same as case (i)
>> (iii) ALL CSR issues are resolved. In this case, we won't add the csr label again and we just need to tell user there already exists resolved CSR issues.
>
> Zhao Song has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   SKARA-1850

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 55:

> 53:         writer.println("@" + pr.author().username() + " this pull request must refer to an issue in " +
> 54:                       "[JBS](https://bugs.openjdk.org) to be able to link it to a [CSR](https://wiki.openjdk.org/display/csr/Main) request. To refer this pull request to " +
> 55:                       "an issue in JBS, please use the `/issue` command in a comment in this pull request.");

This is unrelated to your change, but these instructions are wrong. The main issue must be set through the title and not the /issue command.
Suggestion:

                      "an issue in JBS, please update the title of this pull request to just the issue ID.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 60:

> 58:     private static void linkReply(PullRequest pr, PrintWriter writer) {
> 59:         writer.println("@" + pr.author().username() + " please create a [CSR](https://wiki.openjdk.org/display/csr/Main) request for any issue this pr solves" +
> 60:                 " with the correct fix version. This pull request cannot be integrated until the CSR request is approved.");

The name of this method `linkReply` implies that it's linking to the issue for which the CSR is needed. I think we should still do that in the normal case where there is only one issue, but we need to use the SolvesTracker to figure out if there are multiple issues first. When there are multiple issues, we can print a different message. I would suggest something like this:

Suggestion:

        writer.println("@" + pr.author().username() + " please create a [CSR](https://wiki.openjdk.org/display/csr/Main) request, with the correct fix version, for at least one of the issues associated with this pull request." +
                " This pull request cannot be integrated until all the CSR request are approved.");

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 106:

> 104:             var csrLinks = new StringBuilder();
> 105:             for (Matcher csr : csrs) {
> 106:                 csrLinks.append("[").append(csr.group(1)).append("](").append(csr.group(2)).append(")").append(" ");

I think this can be moved into the conditional block below.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 154:

> 152:         var csrLinks = new StringBuilder();
> 153:         for (Matcher resolvedCSR : resolvedCSRs) {
> 154:             csrLinks.append("[").append(resolvedCSR.group(1)).append("](").append(resolvedCSR.group(2)).append(")").append(" ");

Same here, this could move into the if block below.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1180:

> 1178:             }
> 1179: 
> 1180:             var confFile = localRepo.show(Path.of(".jcheck/conf"), localHash);

I recommend using `localRepo.lines` here instead of `show`, then you don't need to be responsible for knowing the encoding of the file contents on line 1183.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 116:

> 114:                     .flatMap(comment -> comment.body().lines())
> 115:                     .filter(line -> line.startsWith("/csr"))
> 116:                     .collect(Collectors.joining());

I think we only need to look for the last `CSR_NEEDED_MARKER` or `CSR_UNNEEDED_MARKER`. Unless the csr needed state changes from the /csr command, is there anything for the `CheckRun` to update? Reacting to the user issuing `/csr` seems wrong to me because we need the `PullRequestCommandWorkItem` to have finished processing it before `CheckRun` can find anything to act on.

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1496#discussion_r1158937825
PR Review Comment: https://git.openjdk.org/skara/pull/1496#discussion_r1158942324
PR Review Comment: https://git.openjdk.org/skara/pull/1496#discussion_r1158932939
PR Review Comment: https://git.openjdk.org/skara/pull/1496#discussion_r1158934935
PR Review Comment: https://git.openjdk.org/skara/pull/1496#discussion_r1158949534
PR Review Comment: https://git.openjdk.org/skara/pull/1496#discussion_r1158955095