RFR: 1691: Run Jcheck twice in CheckRun if .jcheck/conf has changed

[Erik Joelsson]

On Wed, 7 Dec 2022 19:44:52 GMT, Zhao Song <zsong at openjdk.org> wrote:

> When a pr is created, pr bot will run jcheck to check whether this commit is good. However, currently, we only run jcheck with the configuration from the target branch. Sometimes, user wants to modify jcheck configuration and accidentally messes up the format. In this case, pr bot would not be able to find this problem. After this commit with badly formatted jcheck configuration integrated, all the jcheck will fail and our bot gets stuck into a retry loop.
> 
> In this patch, if jcheck configuration file is updated, the bot will run jcheck twice, once with the configuration in the target and once with the new one in the source. So if the new configuration has any problem, it will became an integration blocker.

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

> 1081:                 var additionalConfiguration = AdditionalConfiguration.get(localRepo, localHash, pr.repository().forge().currentUser(), comments);
> 1082:                 checkablePullRequest.executeChecks(localHash, censusInstance, visitor, additionalConfiguration, true);
> 1083:                 if (localRepo.isFileUpdated(".jcheck/conf")) {

We probably don't need to run the second round if confOverride is set.

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

> 1092:                                 .toList());
> 1093:                     } catch (Exception e) {
> 1094:                         secondJCheckMessage.add(e.getMessage() + "(exception thrown when running jcheck with updated jcheck configuration)");

Can we log this exception?

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

> 207: 
> 208:     void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor,
> 209:                        List<String> additionalConfiguration, boolean useTargetConf) throws IOException {

Instead of a boolean `useTargetConf`, I think it's better if we just supply the hash we want to use (target or src, if confOverride is set, then this method will handle it).

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java line 2729:

> 2727: 
> 2728:             // pr body should have the integrationBlocker for whitespace and reviewer check
> 2729:             assertTrue(pr.store().body().contains("Whitespace errors(failed with the updated jcheck configuration)"));

Please add a space before the `(` for all of these.

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java line 2786:

> 2784:             try (var output = new FileWriter(checkConf.toFile(), true)) {
> 2785:                 output.append("\nRandomCharacters");
> 2786:             }

This could be done with

Files.writeString(checkConf, "text...", StandardOpenOption.APPEND)

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1725:

> 1723: 
> 1724:     @Override
> 1725:     public boolean isFileUpdated(String filename) throws IOException {

This method seems a bit too specialized to me. It should at least take a Hash argument. I think it would be even better to use the existing `commits` method from which you can get to the Diff and Patch objects. That way we have properly parsed and typed data to find the filename in.

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

PR: https://git.openjdk.org/skara/pull/1439