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

[Erik Joelsson]

On Thu, 7 Apr 2022 18:18:34 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

I have looked over the design and the product code and left some comments. I have not yet looked at the tests.

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

> 50:     @Override
> 51:     public boolean concurrentWith(WorkItem other) {
> 52:         if (!(other instanceof JEPBot otherBot))

Please don't omit bracers around conditional blocks.

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

> 48: 
> 49: public class IssueNotifierTests {
> 50:     private static final String JEP_NUMBER = "customfield_10701";

You should be able to import this from JiraProject instead of duplicating the definition.

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

> 156: 
> 157:     private Optional<Issue> getJepIssue() {
> 158:         var issueOpt = getJepIssueFromIssueTracker();

Do we really need to get the issue from the issue tracker at this point? For other issues, CheckRun just gets the ID and description from the encoded marker pattern. See SolvesTracker.java.

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

> 276:                     "Completed".equals(issueStatus) || ("Closed".equals(issueStatus) && "Delivered".equals(resolutionName))) {
> 277:                     ret.put("Change requires a JEP request to be targeted", true);
> 278:                 }

We shouldn't need to repeat the work of the JEPBot here. We should just need to check if there is a comment matching the jepPattern and it's not saying unneeded.

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

> 48:                  * `/jep JEP-<jep-id>`
> 49:                  * `/jep jep-<jep-id>`
> 50:                  * `/jep [unneeded|uneeded]`

The command should accept the alternate spelling 'uneeded' but we should not list it in the help text.

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

> 97:                 reply.println("only [Reviewers](https://openjdk.java.net/bylaws#reviewer) can determine that a JEP request is not needed.");
> 98:                 return;
> 99:             }

I don't think I agree with this check. If the author is able to link the PR to a JEP, then the author should be able to correct a mistakenly linked JEP.

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

> 154:     @Override
> 155:     public String description() {
> 156:         return "require a JDK enhancement proposal for this pull request";

Suggestion:

        return "require a JDK Enhancement Proposal for this pull request";

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

> 42: 
> 43: class CheckTests {
> 44:     private static final String JEP_NUMBER = "customfield_10701";

It should be possible to import this from JiraProject as well.

issuetracker/src/main/java/org/openjdk/skara/issuetracker/jira/JiraProject.java line 438:

> 436: 
> 437:     @Override
> 438:     public Optional<Issue> jepIssue(String jepId) {

Have you verified that this works?

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

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