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

[Guoxiong Li]

On Fri, 8 Apr 2022 17:52:10 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixes code according to the PR comments.
>
> 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.

Fixed.

> 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.

Fixed.

> 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.

Fixed.

> 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.

Fixed.

> 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.

Fixed.

> 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.

The author can use the `/jep <jep-id>|<issue-id>` again if the author linked the wrong JEP. This check follows the convention of the `csr` command. It can protect the situation that the author uses the command `/jep unneeded` and integrates the patch unexpectly.

Actually, the `/jep unneeded` command is rarely used and the author can request the reviewers to do that if the patch really don't need a JEP to be targeted. Preventing the unexpected (and maybe wrong) code from integrating should be the most important thing we need to ensure even though it has not happened in the past.

> 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";

Fixed.

> 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.

Fixed.

> 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?

Not, I have not.

When I request the link `https://bugs.openjdk.java.net/rest/api/2/search?jql=project = JDK AND customfield_10701 = 11` locally, it reports `{"errorMessages":["Field 'customfield_10701' does not exist or you do not have permission to view it."],"errors":{}}`.

But when I request the link `https://bugs.openjdk.java.net/rest/api/2/search?jql=project = JDK AND type = JEP`, it returns the correct data.

It seems that my account don't have permission to search the field `customfield_10701`. Could I get your help to verify it?

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

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