RFR: 1393: PR bot fails on missing .jcheck/conf [v2]

Erik Joelsson erikj at openjdk.org
Tue Nov 1 19:56:59 UTC 2022


On Tue, 1 Nov 2022 18:41:45 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   SKARA-1217
>
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 243:
> 
>> 241:                         + "Until that is resolved, this pull request cannot be processed. Please notify the repository owner.";
>> 242:                 addErrorComment(text, comments);
>> 243:             }
> 
> I think this is another situation which is that the override configuration is invalid or missing

If we throw two separate exception types as suggested below, then we can provide more detailed error messages, for either a missing or an invalid jcheck conf.

In the case of the override jcheck configuration, the file can be configured to have any name. I would suggest:
"The external jcheck configuration for this repository could not be resolved. Until that is fixed, this pull request cannot be processed. Please notify a Skara admin."

> bots/pr/src/main/java/org/openjdk/skara/bots/pr/LimitedCensusInstance.java line 47:
> 
>> 45:                     repository, ref, confOverrideRepo, confOverrideName, confOverrideRef);
>> 46:             if (configuration.isEmpty()) {
>> 47:                 throw new InvalidJCheckConfException(".jcheck/conf is invalid or missing");
> 
> Hi Erik, I don't know if I understood you exactly to throw the exception here.

I think it would fit better in `configuration` as an `.orElseThrow()`. In that case it's only thrown if jcheck is missing, so the exception should be named MissingJCheckConfException.

The actual parsing of the configuration seems to only explicitly throw IllegalArgumentException. It's possible that other parse problems would result in other runtime exceptions, such as NPE. I think we should wrap the parse call with try-catch for RuntimeException and throw an `InvalidJCheckConfException` if that happens.

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

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


More information about the skara-dev mailing list