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

[Erik Joelsson]

On Tue, 27 Jun 2023 21:37:35 GMT, Zhao Song <zsong at openjdk.org> wrote:

> As Erik said in the description of this issue, currently, this issue only cares about tracking approval labels in the related bugs.
> 
> If a repository is set up with the "approval" configuration, pull requests in that repository will require the maintainer's approval in JBS. Otherwise, the pull request will not be considered ready. 
> 
> Erik has also provided a design outlining how to configure the "approval" for a repository.
> 
> 
> The simple case, where the labels are the same for every branch in a repository: 
> 
> "approval": { 
>   "request": "jdk17u-fix-request", 
>   "approved": "jdk17u-fix-yes", 
>   "rejected": "jdk17u-fix-no", 
> } 
> 
> To reduce the need for changing multiple strings when copying a configuration for a new repository, there is an optional "prefix" field: 
> 
> "approval": { 
>   "prefix": "jdk17u-fix-", 
>   "request": "request", 
>   "approved": "yes", 
>   "rejected": "no", 
> } 
> 
> When there are multiple branches with different labels, having the prefix set per branch can help reduce the size of the configuration significantly: 
> 
> "approval": { 
>   "request": "-critical-request", 
>   "approved": "-critical-approved", 
>   "rejected": "-critical-rejected", 
>   "branches": [ 
>     "jdk20\.0\.1": { "prefix": "CPU23_04" } 
>   ] 
> }

When I looked at this again, I realized I missed a key functionality in my description. I've updated the bug and added a [comment](https://bugs.openjdk.org/browse/SKARA-1199?focusedCommentId=14592499&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14592499).

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

> 276:                     var labelNames = issue.labelNames();
> 277:                     if (labelNames.contains(approval.approvedLabel(pr.targetRef()))) {
> 278:                         ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval in JBS", true);

No need to reference JBS since we are already linking to the issue tracker with the issue id. In general we should avoid referencing specific issue tracker instances in source code.
Suggestion:

                        ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval", true);

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

> 278:                         ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval in JBS", true);
> 279:                     } else {
> 280:                         ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval in JBS", false);

Suggestion:

                        ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval", false);

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

> 710:                                     status = "Requested";
> 711:                                 }
> 712:                                 if (!status.isEmpty() && !status.isBlank()) {

Checking just for empty should be enough here.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 182:

> 180:                             issueData.append(properties.get("issuetype").asString());
> 181:                         }
> 182:                         issueData.append(String.join("", issue.labelNames()));

This is currently adding another query to the issue tracker, but that can actually be fixed, and I think it's worth doing that in this patch. You can override `Issue::labelNames` in `JiraIssue` and just read the names from the json. There is a labels field with the names in it. I had this in a local patch for a while but it didn't fit with the thing I was working on at the time, and then I forgot.

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

PR Review: https://git.openjdk.org/skara/pull/1537#pullrequestreview-1503019313
PR Review Comment: https://git.openjdk.org/skara/pull/1537#discussion_r1245167449
PR Review Comment: https://git.openjdk.org/skara/pull/1537#discussion_r1245167944
PR Review Comment: https://git.openjdk.org/skara/pull/1537#discussion_r1245176010
PR Review Comment: https://git.openjdk.org/skara/pull/1537#discussion_r1245185515