RFR: SKARA-1096: New command and label for JEPs, similar to CSR [v10]

[Erik Joelsson]

On Wed, 20 Apr 2022 10:52:24 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch implements the features about the `/jep` command and JEP bot. The detailed designs and discussions are listed at [1][2].
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>> 
>> [1] https://mail.openjdk.java.net/pipermail/skara-dev/2021-December/005481.html
>> [2] https://mail.openjdk.java.net/pipermail/skara-dev/2022-March/005770.html
>
> Guoxiong Li has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Polish
>  - Overload the handle method and revert the change of unrelated commands

Looks pretty good now. I focused on logging this time as that's something I will need to trace from this in the future. 

Thanks for fixing the API change with a default method, it turned out a lot better. Thanks Magnus for pushing through. I had missed that the existing method already had a default and was considered optional to implement.

I have added the /open command to the wiki.

bots/jep/src/main/java/org/openjdk/skara/bots/jep/JEPBot.java line 75:

> 73:                     pr.removeLabel(JEP_LABEL);
> 74:                 }
> 75:                 log.info("No jep command found in comment for PR-" + pr.id());

This is going to log for most PRs every time this WorkItem runs. I would like to lower it to `fine` level. Also, please see CSR bot for a good way of logging pr name/IDs. I would recommend copying the describe method here.

bots/jep/src/main/java/org/openjdk/skara/bots/jep/JEPBot.java line 82:

> 80:             if ("unneeded".equals(issueId)) {
> 81:                 if (pr.labelNames().contains(JEP_LABEL)) {
> 82:                     pr.removeLabel(JEP_LABEL);

Please log that action was taken, see example below.

bots/jep/src/main/java/org/openjdk/skara/bots/jep/JEPBot.java line 84:

> 82:                     pr.removeLabel(JEP_LABEL);
> 83:                 }
> 84:                 log.info("Found `/jep unneeded` command");

Please include the `describe(pr)` in the log message.

bots/jep/src/main/java/org/openjdk/skara/bots/jep/JEPBot.java line 116:

> 114:                 pr.removeLabel(JEP_LABEL);
> 115:             } else if (!hasTargeted && !pr.labelNames().contains(JEP_LABEL)) {
> 116:                 pr.addLabel(JEP_LABEL);

Here we are taking action, I would like this logged at info level. Something like this:
Suggestion:

                log.info("JEP issue " + issue.id() + " found in state " + issueStatus + ", removing JEP label from " + describe(pr);
                pr.removeLabel(JEP_LABEL);
            } else if (!hasTargeted && !pr.labelNames().contains(JEP_LABEL)) {
                log.info("JEP issue " + issue.id() + " found in state " + issueStatus + ", adding JEP label to " + describe(pr);
                pr.addLabel(JEP_LABEL);

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java line 154:

> 152:             for (var label : labelsToAdd) {
> 153:                 if (!pr.labelNames().contains(label)) {
> 154:                     pr.addLabel(label);

Could you add an info log about adding and removing labels to the PR.

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

PR: https://git.openjdk.java.net/skara/pull/1297