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

[Erik Joelsson]

On Mon, 11 Apr 2022 19:39:03 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 one additional commit since the last revision:
> 
>   Disable the test in JiraProjectTests.

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

> 69:             if (jepComment == null) {
> 70:                 if (pr.labelNames().contains(JEP_LABEL)) {
> 71:                     pr.removeLabel(JEP_LABEL);

There is a potential race here. The /jep command adds the label first and the comment after. Not sure how to solve that. Ideally the label should be added after the comment.

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

> 87:             if (issueOpt.isEmpty()) {
> 88:                 if (pr.labelNames().contains(JEP_LABEL)) {
> 89:                     pr.removeLabel(JEP_LABEL);

If we remove the label in this situation, then the PR bot will mark the PR as `[x] Change requires a JEP request to be targeted`, which isn't true. I think the label should stay on. The jep command will not add the label unless it finds the JEP. I can imagine the following situations where this could happen:

* Jira is having a temporary outage - In this case we should do nothing.
* The JEP is no longer visible (e.g. made oracle confidential) - The user/author needs to decide how to handle this, by referencing another JEP or /jep unneeded.
* The JEP comment is corrupted - I don't think we can expect the bot to figure out this on its own.

Since this is an error state that shouldn't be possible to reach from normal operation, we should log SEVERE so that an admin gets notified. The same goes for the case below where the issue is of the wrong type.

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

> 141:     @Override
> 142:     public String name() {
> 143:         return JEP_LABEL;

Suggestion:

        return CSRBotFactory.NAME;

bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java line 504:

> 502: 
> 503:             var csrIssueComments = updatedJepIssue.comments();
> 504:             assertEquals(0, csrIssueComments.size());

Suggestion:

            var jepIssueLinks = updatedJepIssue.links();
            assertEquals(0, jepIssueLinks.size());

            var jepIssueComments = updatedJepIssue.comments();
            assertEquals(0, jepIssueComments.size());

bots/pr/src/main/java/org/openjdk/skara/bots/pr/JEPCommand.java line 139:

> 137:             }
> 138:         } else if ("Draft".equals(issueStatus) || "Submitted".equals(issueStatus) || "Candidate".equals(issueStatus) ||
> 139:                 "Proposed to Target".equals(issueStatus) || "Proposed to Drop".equals(issueStatus) || "Closed".equals(issueStatus)) {

I think this should either just be "else", or we need an else clause after this for catching any unknown states. I think I prefer the former.

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

> 1213:             var mainIssue = issueProject.createIssue("The main issue", List.of("main"), Map.of("issuetype", JSON.of("Bug")));
> 1214:             var jepIssue = issueProject.createIssue("The jep issue", List.of("Jep body"),
> 1215:                     Map.of("issuetype", JSON.of("JEP"), "status", JSON.object().put("name", "Submitted"), JiraProject.JEP_NUMBER, JSON.of("123")));

Consider using static import for JEP_NUMBER.

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

> 1246:             assertTrue(pr.body().contains("### Issues"));
> 1247:             assertTrue(pr.body().contains("The main issue"));
> 1248:             assertTrue(pr.body().contains("The jep issue (**JEP**)"));

This part of the test isn't really doing anything. Should it perhaps remove the JEP label and verify that the JEP requirement is now met?

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

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