RFR: 1385: Backport MR links to the wrong CSR request [v2]

[Erik Joelsson]

On Mon, 9 May 2022 11:28:29 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch solves the wrong CSR in the backport PR. It mainly has the following rules:
>> 1. if the `version` in `.jcheck/conf` doesn't exist or the `version` is wrong, the bot think the CSR also doesn't exist.
>> 2. else, if the `version` in `.jcheck/conf` matches the fix version of the primary CSR, the bot will use the primary CSR. (primary CSR is the CSR of the primary issue)
>> 3. else, if these is a backport issue matches the `version`, the bot will find the CSR of the backport issue. If found, use it. If not, the bot think the CSR doesn't exist.
>> 4. else (no backport issue matches the `version`) the bot think the CSR doesn't exist.
>> 
>> The classes `CSRBot`, `CheckRun` and `CSRCommand` will handle the situations above respectively.
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix the complex situation.

I've looked through and added some comments. This can be a tricky feature to get right so please be patient.

bots/csr/src/main/java/org/openjdk/skara/bots/csr/CSRBot.java line 110:

> 108:             var jbsIssue = jbsIssueOpt.get();
> 109:             var csrOptional = csrLink(jbsIssue).flatMap(Link::issue);
> 110:             if (csrOptional.isEmpty()) {

I don't think we can require a CSR to exist on the main bug to check for CSRs on the backport issues.

bots/csr/src/main/java/org/openjdk/skara/bots/csr/CSRBot.java line 116:

> 114: 
> 115:             var csr = csrOptional.get();
> 116:             if (pr.labelNames().contains("backport")) {

I don't think we should only check backports when the label is set. It's currently not a requirement to create PRs as backport PRs. I think we should be more proactive and only accept CSR issues that have a matching fixVersion, and always look for them in the whole backport tree.

As we are now getting smarter about fixversion in the PR bot, as a followup fix, we could consider no longer complaining about the main issue being closed, if a backport exists with the correct fixVersion.

bots/csr/src/main/java/org/openjdk/skara/bots/csr/CSRBot.java line 119:

> 117:                 csrOptional = findBackportCsr(pr, jbsIssue, csr);
> 118:                 if (csrOptional.isEmpty()) {
> 119:                     log.info("Not found backport CSR for " + describe(pr));

Suggestion:

                    log.info("No backport CSR found for " + describe(pr));

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

> 172:         if (jdkVersion.isEmpty()) {
> 173:             return Optional.empty();
> 174:         }

This block of code is repeated 3 times now. I know we don't have a good place to share it between different bots, but we should be able to put it in a single location for CSRCommand and CheckRun at least.

jbs/src/main/java/org/openjdk/skara/jbs/IssueUtil.java line 45:

> 43:      * A "scratch" fixVersion is empty, "tbd.*", or "unknown".
> 44:      */
> 45:     public static Optional<Issue> findClosestIssue(List<Issue> issueList, JdkVersion fixVersion) {

I think this method belongs in the Backports class, close to Backports#findIssue. They do almost the same thing, so if we are to maintain two similar implementations of the version matching order, they should reside close to each other.

Could we move more of the common functionality to Backports. Something like `Backports.findCsr(Issue primary, JdkVersion jdkVersion)` that would encapsulate most of this repeated functionality?

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

PR: https://git.openjdk.java.net/skara/pull/1318