RFR: 2082: Use correct .jcheck/conf in AdditionalConfiguration.java [v5]

[Erik Joelsson]

On Thu, 30 Nov 2023 19:34:32 GMT, Erik Duveblad <ehelin at openjdk.org> wrote:

>> Hi all,
>> 
>> please review this patch that ensures that we pass the correct `.jcheck/conf` to [AdditionalConfiguration.java](https://github.com/openjdk/skara/blob/master/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AdditionalConfiguration.java). The `.jcheck/conf` should either come from the "overriding" `.jcheck/conf` (if the PR bot is configured to use that) or the pull request's target branch.
>> 
>> I have added that test that passes with this patch but fails without it.
>> 
>> Thanks,
>> Erik
>
> Erik Duveblad has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
> 
>  - Merge 'master'
>  - Reviewer feedback
>  - Merge master
>  - optimize
>  - final review
>  - refactor
>  - whitespace
>  - do not throw from checkStatus
>  - Merge branch 'master' into pr-correct-jcheck-conf
>  - Fix .jcheck/conf parsing in multiple places
>  - ... and 1 more: https://git.openjdk.org/skara/compare/a9727b65...f22f4878

I like how this is shaping out. Just some minor issues left.

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

> 1251: 
> 1252:                         var commitJCheckConf = checkablePullRequest.parseJCheckConfiguration(hash);
> 1253:                         var commitVisitor = checkablePullRequest.createVisitor(commitJCheckConf);

I think these should be moved inside the if block below.

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

> 1252:                         var commitJCheckConf = checkablePullRequest.parseJCheckConfiguration(hash);
> 1253:                         var commitVisitor = checkablePullRequest.createVisitor(commitJCheckConf);
> 1254:                         if (isJCheckConfUpdatedInMergePR) {

Shouldn't this be conditional on the repo using an external jcheck conf?

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

> 1302:                 // then we won't use the one in the repo anyway.
> 1303:                 if (workItem.bot.confOverrideRepository().isEmpty() &&
> 1304:                     (isFileUpdated(Path.of(".jcheck").resolve("conf"), localHash) || isJCheckConfUpdatedInMergePR)) {

Style nit. I think this way of creating a path looks a bit overly complex. I would suggest either just `Path.of(".jcheck/conf")` or split the elements like this `Path.of(".jcheck", "conf")`.

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

> 1306:                     var localJCheckConf = checkablePullRequest.parseJCheckConfiguration(localHash);
> 1307:                     var localVisitor = checkablePullRequest.createVisitor(localJCheckConf);
> 1308:                     log.info("Run jcheck against localHash with JCHeck configuration from localHash");

Suggestion:

                    log.info("Run JCheck against localHash with configuration from localHash");

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

> 1318:             }
> 1319: 
> 1320:             var confFile = localRepo.lines(Path.of(".jcheck").resolve("conf"), localHash);

Same style nit as above.

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

PR Review: https://git.openjdk.org/skara/pull/1578#pullrequestreview-1759864698
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412135881
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412152819
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412143916
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412154904
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1412156967