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

[Guoxiong Li]

On Mon, 9 May 2022 13:31:17 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix the complex situation.
>
> 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.

It is necessary. The primary CSR may has the fix versions of the backport issues. 

Such as this CSR:
https://bugs.openjdk.java.net/browse/JDK-8285500

> 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.

 I cleaned the condition and now the code always searches all the related CSRs for the right fix version.

> we could consider no longer complaining about the main issue being closed

I don't understand it. Could you point the related code or give more information?

> 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));

Fixed.

> 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.

I add a method `findCsr(Issue primary, JdkVersion version)` to the class `IssueUtil`. This method finds all the backport issues according to the primary issue and then gets all the CSRs and finally finds the needed CSR by using the method `findClosestIssue`.

> 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.

The `Backports` class mainly handles something about backport. But this method is only about issue instead of backport. 
Actully, the method `Backports#findIssue` should delegate to a method such as `IssueUtil#findClosestIssue(List<Issue> issueList, JdkVersion fixVersion, boolean onlySearchMainFixVersion)` after getting the backport issues to find the closest issue by using the main fix version. Such refactor is good but I don't think it should be done at this patch. So I would like to keep the utilility method about issues at a seperate class so that we can do more refactor in the future. The class name `IssueUtil` may need to be adjusted to a better name.

> 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?

I moved the method `findCsr` and `csrLink` to the class `IssueUtil`. Again, the class name `Backports` is not good to place such methods.

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

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