RFR: 1199: Enforce maintainer approval in JBS before allowing to integrate backports into updates projects [v6]

[Magnus Ihse Bursie]

On Wed, 21 Sep 2022 05:44:04 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch implements the feature we had discussed at the mail list and the issue SKARA-1199 [1], included mainly the proposal I submitted [2] and the suggestions and ideas other developers provided.
>> 
>> The dev flow has been stated at my previous proposal [2]. Here, I mainly state the code implementation which are changed and  newly added in this patch.
>> 
>> ### The change in PullRequestBot
>> - The config about approval and update change is added to the `PullRequestBot`. See below for the concrete config format. Please see the classes `PullRequestBotFactory`, `PullRequestBotBuilder` and `PullRequestBot` for the code.
>> 
>> 
>> "repositories" : {
>>   "repo-name" : {
>>     // other fileds, ignored
>>     "approval" : [
>>       {
>>         "branch" : "branch-pattern",
>>         "request-label" : "label-name1",
>>         "approval-label" : "label-name2",
>>         "disapproval-label" : "label-name3",
>>         "maintainers" : ["user1", "user2"]
>>       },
>>       // other branch pattern and its information
>>     ]
>>   },
>>   // other repo and its information
>> }
>> 
>> 
>> - A suggestion comment would be added to the update change pull request. Please see the method `CheckRun#addUpdateChangeSuggestionComment`.
>> - Labels of the pull request and the issues would be updated according to the main issue state. Please see the method `CheckRun#updateLabelsForUpdatesChange`
>> - Two commands `approval` and `request-approval` are added. Please see the classes `ApprovalCommand` and `RequestApprovalCommand`. The field `CommandExtractor#commandPattern` is also adjusted to identify the bar `-` in `request-approval`.
>> - Some methods in class `CheckRun` are moved to class `PullRequestWorkItem` to let them reused in `PullRequestCommandWorkItem` and `CheckWorkItem`.
>> - The `PullRequestWorkItem workItem` parameter is added to the method `CommandHandler#handle` so that the commands can use the work item and use the newly added and moved methods. And the `handle` method of the `CSRCommand` and `JEPCommand` need to adjust the parameter, too.
>> - The field `CheckWorkItem#metadataComments` need to be adjusted to identify the tag `<!-- apprvoal:` and `<!-- request-approval:` so that the CheckWorkItem can be re-run.
>> 
>> ### The change in the newly added approval module
>> 
>> - The config about the bots in approval module is shown below. Please see the class `ApprovalBotFactory` for the code.
>> 
>> 
>> "projects" : [
>>   {
>>     "repository" : "repo-name",
>>     "issues" : "issue-project-name",
>>     "approval" : [
>>       {
>>         "branch" : "branch-pattern",
>>         "request-label" : "label-name1",
>>         "approval-label" : "label-name2",
>>         "disapproval-label" : "label-name3",
>>         "maintainers" : ["user1", "user2"]
>>       },
>>       // other branch pattern and its information
>>     }
>>   ],
>>   // other repo/project and its information
>> ]
>> 
>> - Two bots `ApprovalPullRequestBot` and `ApprovalIssueBot` are added. Just like the csr bots, the bot `ApprovalPullRequestBot` monitors the pull request updates and another bot monitors the issue updates.
>> - Two work items `ApprovalIssueWorkItem` and `ApprovalPullRequestWorkItem` are added. The `ApprovalIssueWorkItem` gets the update issue and then schedules the `ApprovalPullRequestWorkItem`. The `ApprovalPullRequestWorkItem` monitors the approval or disapproval labels of the main issue, if the corresponding label found, it will add the update marker to the pull request to re-run the CheckWorkItem of the PullRequestBot.
>> 
>> ### Other changes
>> - Two pollers `UpdatedIssuePoller` and `UpdatedPullRequestPoller` are added to the `issuetracker` module and `forge` module. They are extracted from `csr` module to reused in the future (currently used in `approval` module).
>> - A new class `ApprovalInfo` is added to the `bot` module to reused in `approval` module and `pr` module.
>> - The gradle setting and the module info setting are added or changed.
>> -  All the change are covered by the newly added tests and the existing tests.
>> 
>> ### No change in this patch
>> - Although two pollers are extracted from the `csr` module to re-used. But the `csr` and `jep` module are not changed because it would improved the complexity of this patch. The `csr` and `jep` module can be adjusted in the follow-up patches. And the issue SKARA-1554 [3] about the `jep` module has been created.
>> - The API of the `HostedRepository` is not changed. But I add some `TOOD` comments to the class `HostedRepository` and the method `UpdatedPullRequestPoller#getUpdatedPullRequests` to improve the API of the `HostedRepository` in the follow-up patch.
>> - And I found some bugs during completing this patch. All of the issues about the bugs have been created in the JBS.
>> 
>> The patch is so large. Thanks for taking the time to review.
>> 
>> Best Regrads,
>> -- Guoxiong
>> 
>> [1] https://bugs.openjdk.org/browse/SKARA-1199
>> [2] https://mail.openjdk.org/pipermail/jdk-updates-dev/2022-August/016451.html
>> [3] https://bugs.openjdk.org/browse/SKARA-1554
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix tests.

I've (finally) gotten to look at this patch. I can't add much more than what Erik has already said, just a few remarks on the UX aspect of this patch.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 39:

> 37:     private static void showHelp(PrintWriter writer) {
> 38:         writer.println("""
> 39:                 usage: `/approval [yes|no|y|n]`

Perhaps a minor issue, but I think the command should be `/approve` instead of `/approval`. We have verbs in the other commands, such as `/sponsor` and `/integrate`.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 129:

> 127:             });
> 128:         }
> 129:         if (pr.labelNames().contains("approval")) {

Also, the label should probably be `approved`, since we use adjectives (like `ready`) for labels.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandExtractor.java line 66:

> 64:             Map.entry("backport", new BackportCommand()),
> 65:             Map.entry("approval", new ApprovalCommand()),
> 66:             Map.entry("request-approval", new RequestApprovalCommand()),

... and just to be clear: `/request-approval` is an okay command, since "request" is a verb.

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

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