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

[Erik Joelsson]

On Fri, 3 Nov 2023 13:10:52 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 incrementally with one additional commit since the last revision:
> 
>   optimize

Overall I really like this change. It makes dealing with jcheck configs a lot clearer.

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

> 1236:                         var commitJCheckConf = checkablePullRequest.parseJCheckConfiguration(hash)
> 1237:                                                 .orElseThrow(() -> new IllegalStateException("No .jcheck/conf present in tree for commit " + hash));
> 1238:                         var commitVisitor = checkablePullRequest.createVisitor(commitJCheckConf, hash);

We could move the creation of these two into the if block below. Could create `messageStream = targetVisitor.messages().stream()` before the if and override it with the concatenated stream inside the block. I think it's worth trying to minimize any external calls or processes when possible to speed up WorkItem processing.

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

> 1270:             }
> 1271: 
> 1272:             var visitor = checkablePullRequest.createVisitor(targetJCheckConf, localHash);

Supplying the `localHash` here seems like a dummy. It's not used for anything, because the overriding conf will be used regardless, but the current API requires a valid hash here. It took me a while and a bit of confusion before I realized this. Would it be possible to avoid that by getting rid of the unused parameter?

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java line 212:

> 210:     }
> 211: 
> 212:     Optional<JCheckConfiguration> parseJCheckConfiguration(Hash hash) throws IOException {

Every caller of this method is throwing IllegalStateException if jcheck conf isn't found. Did you consider just throwing here and not return Optional? I realize there is a bit more information in the exception message, but on the other hand, whoever reads it will have a stacktrace anyway.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckablePullRequest.java line 228:

> 226:     }
> 227: 
> 228:     void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor, JCheckConfiguration conf) throws IOException {

The parameter name `localHash` threw me off for a bit, maybe just `hash` in this context, unless I'm missing some context?

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

PR Review: https://git.openjdk.org/skara/pull/1578#pullrequestreview-1712704346
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1381694967
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1381716442
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1381728237
PR Review Comment: https://git.openjdk.org/skara/pull/1578#discussion_r1381731872