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

[Guoxiong Li]

On Thu, 1 Sep 2022 22:26:06 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

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

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

Fixed.

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

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