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

Erik Joelsson erikj at openjdk.org
Thu Sep 1 22:29:25 UTC 2022

On Wed, 31 Aug 2022 10:46:32 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

Thank you for taking on this issue. This is a big feature, and I'm going to be pretty adamant that it gets right before it goes in. Thanks in advance for understanding that. 

We need a clearer separation of what the different actors are supposed to be doing. We have the commands, `CheckWorkItem`, `ApprovalsIssueWorkItem` and `ApprovalsPullRequestWorkItem`. I know this was mostly based on the current model of the CSR functionality, but unfortunately, I don't think we are done figuring out how that really ought to be modeled to work well, and because of that, we really shouldn't be expanding more functionality around the current model.

Here are my thoughts on the current Skara bot model and what the actors for approvals are and should be doing:

The commands should just add labels and a specific type of comment to the (at least for now) head issue of a PR. No other actor should be touching the Issues. We may consider having the command automatically/optionally interact will all issues of a PR. When a command is run, it will already automatically trigger a new CheckWorkItem to update the PR state based on the command action.

The role of a CheckWorkItem is to update the (Skara) state of a PR based on other updates to the PR. With that I mean that it polls for PR updates only, no external ones. With regards to approvals, it should be responsible for toggling the `approval` label, updating the progress checkbox and post the information message about the approval process. It should most definitely not be changing any labels in any of the Issues.

The ApprovalsPullRequestWorkItem seems redundant to me. The reason we had a separate bot for CSR was that the CheckWorkItem could only trigger on PR updates. The original CSRBot would just always look at all PRs, but at least only those with the CSR label, which was a small subset (but this still wasn't a good model). When this was expanded to always look at all PRs, it got out of hand, which is when I introduced Issue polling. What we need for approvals is ApprovalsIssueWorkItem (but we can rename it ApprovalsWorkItem), which triggers on Issue updates. It can then do some basic checks (e.g. issue is open, has at least one of the labels, PR has approval label) and if relevant, post the marker to trigger another CheckWorkItem. I think this would be the least risky way of implementing approvals support within the current model.

For a long term solution, I'm thinking that we probably did the wrong thing from the start with introducing a separate CSR, JEP and Approvals bot. What we need is an IssueBot in the pr module that can monitor issues and spawn CheckWorkItems for associated PRs, without going the roundabout way of posting markers. This may be slightly problematic, because there is an inherent dependency on the NotifyBot, which is the one adding the associating link from the Issue back to the PR, so we would need to find a way to work around that, but that's for a separate future change.

bots/approval/src/main/java/org/openjdk/skara/bots/approval/AbstractApprovalBot.java line 48:

> 46:     }
> 47: 
> 48:     boolean isUpdateChange(PullRequest pr) {

The name of this method confuses me. I think something like 'requiresApproval` would be better. The fact that this is currently (only) done for JDK update releases doesn't need to be reflected our method names.

There are several other methods and fields that use the term `updateChange`. I think all of them should be renamed to something about approvals instead.

bots/approval/src/main/java/org/openjdk/skara/bots/approval/ApprovalIssueBot.java line 55:

> 53:         for (var issue : poller.getUpdatedIssues(IssueProject::issues)) {
> 54:             var issueWorkItem = new ApprovalIssueWorkItem(this, issue);
> 55:             log.fine("Scheduling: " + issueWorkItem);

Since SKARA-1552, new WorkItems are logged in the BotRunner, making this log message redundant. This goes for all "scheduling" messages in getPeriodicItems, so I won't comment on each one.

bots/approval/src/main/java/org/openjdk/skara/bots/approval/ApprovalPullRequestWorkItem.java line 86:

> 84:     private boolean hasApprovalProgress(PullRequest pr) {
> 85:         var statusMessage = getStatusMessage(pr);
> 86:         return statusMessage.contains("- [ ] Change must be properly approved by the maintainers") ||

This string is repeated a lot. We really need to centralize it at least within this class. I have filed SKARA-1547 for moving PR constants to a single central location, so I'm ok with one copy per bot for now, but if you want to introduce a `PullRequestConstants` class in the forge module right now and put your new constants there, that would be ok too.

On the other hand, I don't think we need this particular string in this bot.

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

> 55:     private void disapprovalReply(PullRequest pr, PrintWriter writer) {
> 56:         writer.println(String.format("@%s this pull request was rejected by the maintainer. "
> 57:                 + "The bot will close this pull request automatically.", pr.author().username()));


                + "This pull request will be closed.", pr.author().username()));

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

> 63:                        CommandInvocation command, List<Comment> allComments, PrintWriter reply, PullRequestWorkItem workItem) {
> 64:         if (!workItem.isUpdateChange()) {
> 65:             reply.println("this repository or the target branch of this pull request have not been configured to use the `approval` command.");


            reply.println("the `approval` command can only be used on pull requests targeting branches and repositories that require approval.");

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

> 91:                     if (issue.labelNames().contains(workItem.approvalLabelName())) {
> 92:                         // If the maintainers have approved the PR before,
> 93:                         // the bot should remove the approval label at first.


                        // the bot should remove the approval label first.

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

> 107:         }
> 108: 
> 109:         pr.setState(Issue.State.OPEN);

Why change the state of the PR?

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

> 119:                 if (issue.labelNames().contains(workItem.disapprovalLabelName())) {
> 120:                     // If the maintainers have disapproved the PR before,
> 121:                     // the bot should remove the disapproval label at first.


                    // the bot should remove the disapproval label first.

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

> 67:     // The `update change` is usually `backport change`.
> 68:     private static final String updateChangeSuggestionMarker = "<!-- Update change pull request suggestion -->";
> 69:     private static final String APPROVAL_PROGRESS = "Change must be properly approved by the maintainers";

I would suggest a different wording: "All issues must be approved by a maintainer", possibly with the word 'approved' being a link to https://openjdk.org/projects/jdk-updates/approval.html.

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

> 195:             var issue = issueOpt.flatMap(value -> issueProject() != null ? issueProject().issue(value.shortId()) : Optional.empty());
> 196:             if (issue.isPresent()) {
> 197:                 if (issue.get().labelNames().contains(workItem.approvalLabelName())) {

We need to check all the issues to make this call. That should only be done once for the CheckRun, and that result needs to be used both for updating the label as well as the progress.

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

> 202:                 }
> 203:             } else {
> 204:                 // The PR doesn't contain a right issue.


                // The PR doesn't have an issue.

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

> 957:         var existing = findComment(comments, updateChangeSuggestionMarker);
> 958:         if (existing.isPresent()) {
> 959:             // Only add the comment once per PR

Maybe consider updating it if it's different from the existing one. If we were to change the message in the future, it would be nice if active PRs were updated.

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

> 980:                 after applying it. The risk of this backport is low because the change is little and the \
> 981:                 issue fixed by this change also exists in other update repositories.
> 982:                 ```

This is too detailed. The process is officially documented here: https://openjdk.org/projects/jdk-updates/approval.html. We should just link to that and add the additional information about PR commands that can support the process. Maintaining a copy of the process description in the Skara source code is not something I want to do. Something like:

This pull request requires [maintainer approval](https://openjdk.org/projects/jdk-updates/approval.html).

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

> 984:                 If you don't have permission to add a comment in JBS. Please use the command `request-approval` \
> 985:                 to provide the related content, then the bot can help you add a comment by using the content \
> 986:                 you provided. Below is an example for you:


                As a convenience, or if you don't have permission to post comments in JBS, \
                you may use the command `/request-approval` to supply the fix request comment \
                and automatically set the correct request label.
                usage: `/request-approval <comment-text>`

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

> 991:                 after applying it. The risk of this backport is low because the change is little and the \
> 992:                 issue fixed by this change also exists in other update repositories.
> 993:                 ```

I don't think we should post an example here.

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

> 997:                 And you don't need to add the fix request label manually to the issue like before, \
> 998:                 now the bot can help you add the label automatically when this pull request is ready \
> 999:                 for maintainers to approve.


                Please note that approval discussions should take place in the issue and not in the pull request.

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

> 1193:                                            .filter(entry -> !entry.getKey().equals(APPROVAL_PROGRESS))
> 1194:                                            .allMatch(Map.Entry::getValue) &&
> 1195:                                        integrationBlockers.isEmpty();

Could we share the common parts of these logic expressions?


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

More information about the skara-dev mailing list