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

[lgxbslgx]

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

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace

Issue

• SKARA-1199: Enforce maintainer approval in JBS before allowing to integrate backports into updates projects

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara pull/1364/head:pull/1364
$ git checkout pull/1364

Update a local copy of the PR:
$ git checkout pull/1364
$ git pull https://git.openjdk.org/skara pull/1364/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1364

View PR using the GUI difftool:
$ git pr show -t 1364

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1364.diff

[bridgekeeper[bot]]

:wave: Welcome back gli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge[bot]]

Webrevs

• 06: Full - Incremental (970c38e8)
• 05: Full - Incremental (dcc2d568)
• 04: Full (1ce111ff)
• 03: Full - Incremental (433533d6)
• 02: Full - Incremental (24fa9fa4)
• 01: Full - Incremental (5ad7200c)
• 00: Full (d9bc6154)

[lgxbslgx]

The test UpdatedIssuePollerTest#testGetUpdatedIssues and UpdatedIssuePollerTest#testGetUpdatedCsrIssues failed on Windows platform. It passed locally (linux&x64) and I don't know the failed reason now. And I don't have the windows platform to debug.

I read the log of the Presubmit tests, it seems the UpdatedIssuePollerTest doesn't run in Linux and macOS.

[erikj79]

This is a huge patch and I have started reviewing it. Can't promise when I will be able to post it.

[lgxbslgx]

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.

Filed https://bugs.openjdk.org/browse/SKARA-1576 to follow up.

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.

Filed https://bugs.openjdk.org/browse/SKARA-1575 to follow up.

[lgxbslgx]

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.

As you stated, the command should only change the head issue and the CheckWorkItem shouldn't change the issues. My understanding is: the /approval yes command only adds the approval label to the head issue and the CheckWorkItem doesn't sync the approval label to other issues.

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.

But here, you think we should check all the issues to check the approval label. It seems a contradiction in them. Do I miss something or misunderstand something?

[lgxbslgx]

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.

It is good to reduce the two bots and two work items to one bot and one work item.

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.

After reading your comment, I think it is better to implement the approval feature in the pr module instead of adding the new approval module. Maybe we can begin the work in this patch?(Add the IssueBot and ApprovalWorkItem to the pr module) It seems not hard to achieve.

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.

If we want to refator csr, jep and approval modules into pr module, it seems the NotifyBot can't influence it. Do you want to move the issue related feature from notify module to pr module so that it needs to think more? Or Could you explain more information about it?

[erikj79]

After reading your comment, I think it is better to implement the approval feature in the pr module instead of adding the new approval module. Maybe we can begin the work in this patch?(Add the IssueBot and ApprovalWorkItem to the pr module) It seems not hard to achieve.

No, because then we add the implicit dependency on issue notifications to the PR module internals. I'm not ok with that.

If we want to refator csr, jep and approval modules into pr module, it seems the NotifyBot can't influence it. Do you want to move the issue related feature from notify module to pr module so that it needs to think more? Or Could you explain more information about it?

I do not want to move too much into the pr bot, especially not any notification services. I merely tried to point out why it would be problematic to move Issue polling into the PR bot and that is something that needs to be figured out. I don't have the solution yet. I might have some vague ideas, but I'm not ready to share yet, mainly because I have more urgent issues to focus on atm.

[lgxbslgx]

When the developers use the /issue command to remove an issue, the previously added labels will always exist in that issue and can't be removed. But I can't find a good solution to solve it now. A not good way: move the related code to the notify bot so that the issue removal event can be handled. But the configuration will be copied again which is very bad.

[lgxbslgx]

When the developers use the /issue command to remove an issue, the previously added labels will always exist in that issue and can't be removed. But I can't find a good solution to solve it now. A not good way: move the related code to the notify bot so that the issue removal event can be handled. But the configuration will be copied again which is very bad.

[bridgekeeper[bot]]

@lgxbslgx This pull request has been inactive for more than 3 weeks and will be automatically closed if another 3 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@lgxbslgx This pull request has been inactive for more than 6 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.