RFR: 1912: Show priority for bugs in pull request body

[Erik Joelsson]

On Mon, 22 May 2023 21:53:03 GMT, Zhao Song <zsong at openjdk.org> wrote:

> The initial purpose of this issue is to show priority for bugs in the issue list in the pull request body. However, with current pr bot, if the user changes the type of the issue or the priority of the issue in the JBS, the changes will not be reflected in the PR body unless there is something could trigger the CheckWorkItem(like user edits the PR title and user issues some commands).
> 
> As Erik.J suggested, we could maintain a map(from JBS issue to the PRs related with this issue) in memory, so if the issue gets updated, we could know which pr needs to be updated.
> 
> To query updated issues and handle them, a new bot called `IssueBot` is introduced. This bot would query for updated issues(exclude CSR and JEP) in JBS, and it will read the in memory map to know whether there is any pr needs to be updated. If so, it will generate `CheckWorkItem` for that pr.
> 
> So currently, there are two ways to generate `CheckWorkItem`. Updated issues and updated pull requests. Previously, in the `CheckWorkItem`, we would check the metadata of the pull request and if the metadata is up to date, `jcheck` would not be triggered.  Now, we could check the metadata of the pull request and the issues related to this pr, but it would be too expensive because if only the pull request is updated, we also need to fetch all the issues from JBS to just generate the metadata. Therefore, in this patch, metadata is split to two parts, one part for pull request and one part for issues. If the `CheckWorkItem` is spawned from an updated pull request, we will only check pr metadata. On the other hand, if the `CheckWorkItem` is spawned from an updated issue, we will only check issues metadata.
> 
> Besides, since the map is in-memory, so if the bot restarts, the map needs to be initialized. When bot restarts, a `CheckWorkItem` would be generated for each pr, so the initialization is in `CheckWorkItem`.

This is a big change that will likely take a few rounds of review before we get it right. I have made a first pass and left comments mostly about higher level structural issues.

In addition to the below, I think it may be worth thinking about how we can minimize the amount of times we fetch the same issue from an IssueProject. Can we perhaps cache them in CheckWorkItem to avoid repeated fetching?

(accidental double post)

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRIssueWorkItem.java line 102:

> 100:                 .flatMap(id -> bot.repositories().stream()
> 101:                         .filter(r -> r.name().equals(id.split("#")[0]))
> 102:                         .map(r -> r.pullRequest(id.split("#")[1]))

I would suggest storing a better type than `String` in the issuePRMap so we don't have to parse it. Storing the full PullRequest object is a bit wasteful and could get confusing, but you could create a small `record` with just the repository name and pull request id.

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

> 705:                                 } else {
> 706:                                     progressBody.append(" (⚠️ Uncommon issue type: " + issueType.asString() + ")");
> 707:                                     currentIssues.add(iss.get().id());

This should all just be in one `else` clause where the priority is printed. The issue types `CSR` and `JEP` are the only exceptions where we don't need to print priority.

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

> 322: 
> 323:     private void initializeIssuePRMap() {
> 324:         // When bot restarts, the issuePRMap needs to get updated with this pr

I'm not sure about this comment. This mapping needs to be updated every time there is a change to the associations, or if the map is currently missing associations for this PR. I don't think it makes sense to maintain an `initializedPRs` map, we should just always update the map.

This update process needs to be synchronized. The map is a `ConcurrentHashMap`, which is good, but we need to synchronize on the list value before trying to manipulate it.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/IssueWorkItem.java line 82:

> 80: 
> 81:     @Override
> 82:     public Collection<WorkItem> run(Path scratchPath) {

I don't think we need a separate IssueWorkItem. The reason for the `CSRIssueWorkItem` is that we need to do potentially expensive work for each CSR issue found to translate them into `CheckWorkItem` (follow multiple issue links through queries). In this IssueWorkItem, all we need to do is look up the issue ID in an in-memory map, which is very cheap. That can easily be done in a loop in `bot.getPeriodicItems` without risking it taking too long.

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

PR Review: https://git.openjdk.org/skara/pull/1523#pullrequestreview-1439642252
PR Review: https://git.openjdk.org/skara/pull/1523#pullrequestreview-1440169869
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1202283150
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1202311811
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1202672067
PR Review Comment: https://git.openjdk.org/skara/pull/1523#discussion_r1202680765