RFR: 1707: Bot warns about "No .jcheck/conf found" when it should not [v5]

[Erik Joelsson]

On Tue, 6 Dec 2022 18:10:54 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> In [SKARA-1393](https://bugs.openjdk.org/browse/SKARA-1393), I added some checks to ensure the target branch of a pr contains valid jcheck configuration. This change went live on November 28, however, some users found that the warning of "No .jcheck/conf found" printed after they integrated their pr. After more investigation, I am thinking it's maybe a GitLab bug. Since when the issue happens, GitLab will always return "Commit Not Found", so we could have a temporary workaround right now.
>> 
>> For GitLab REST API:
>> If commit not found, it will return "404 Commit Not Found"
>> 
>> If file not found it will return "404 File Not Found"
>> 
>> For Github REST API:
>> If commit not found, it will return "No commit found for the ref "
>> 
>> If file not found it will return "Not Found"
>
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
> 
>   rename variables

bots/common/src/main/java/org/openjdk/skara/bots/common/BotUtils.java line 46:

> 44:         String confFile;
> 45:         confFile = pr.repository().fileContents(".jcheck/conf", pr.headHash().hex())
> 46:                 .orElse(pr.repository().fileContents(".jcheck/conf", pr.targetRef()).orElseThrow());

Could you add a message to the exception in orElseThrow describing what's happening. Something like

() -> new RuntimeException("Missing .jcheck/conf in both src and target of PR " + pr)


Also, no need to split declaration of confFile to a separate line anymore, nor declare the type.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/LimitedCensusInstance.java line 77:

> 75:             conf = Optional.of(Arrays.stream(contents.get().split("\n")).toList());
> 76:         }
> 77:         return conf.map(JCheckConfiguration::parse);

This seems a bit awkward. I think something like this should do it (untested and likely not completely right):

return remoteRepo.fileContents(name, ref).map(contents -> JCheckConfiguration.parse(Arrays.stream(contents.split("\n")).toList()));

forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java line 276:

> 274:             }
> 275:             throw e;
> 276:         }

Instead of catching this exception, please use the onError method on the request to register an error handler.

forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java line 298:

> 296:                         // Retry once with additional escaping of the path fragment
> 297:                         // Only retry when the error is exactly "File Not Found"
> 298:                         if (response.statusCode() == 404 && response.body().contains("File Not Found")) {

The comment says 'exactly "File Not Found"' but the conditional uses `.contains`. I think we can switch to `.equals`.

forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java line 313:

> 311:                 return Optional.empty();
> 312:             }
> 313:             throw e;

Instead of catching exception, I think it's better to register yet another `.onError` handler on the nested request object. You would then return a special JSONValue instance that you can check for and then the outer method can return `Optional.empty()`. There are other examples of this pattern in this file.

network/src/main/java/org/openjdk/skara/network/UncheckedRestException.java line 13:

> 11:     String body;
> 12: 
> 13:     public UncheckedRestException(String message, int statusCode, String body) {

Even if we don't end up using this extra body field on this exception type, I think it's good to have, so please leave it in.

test/src/main/java/org/openjdk/skara/test/TestHostedRepository.java line 203:

> 201:         try {
> 202:             var lines = localRepository.lines(Path.of(filename), localRepository.resolve(ref).orElseThrow());
> 203:             return Optional.of(String.join("\n", lines.orElseThrow()));

This should be expressed as something like:

return lines.map(content -> String.join("\n", content));

This makes us preserve an Optional.empty() returned from the localRepository.

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

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