RFR: 1851: Fold CSR bot into PR bot

[Erik Joelsson]

On Mon, 27 Mar 2023 21:48:23 GMT, Zhao Song <zsong at openjdk.org> wrote:

> In this patch, the whole csr bot module is removed. 
> 
> Original CSRPullRequestBot is deleted. The functionality of this bot has been moved to `CheckRun`.
> 
> Original CSRIssueBot has been moved to pr bot module. PullRequestBotFactory would generate a CSRIssueBot for every configured Issue project.

I've skimmed through a first pass of the patch. The comments about unifying functionality in CheckRun could perhaps be done in a separate followup fix to keep the size of this change down.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRIssueBot.java line 54:

> 52:         this.pullRequestBotMap = pullRequestBotMap;
> 53:         // The CSRPullRequestBot will initially evaluate all active PRs so there
> 54:         // is no need to look at any issues older than the start time of the bot

This comment needs to be updated.

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

> 1285:     }
> 1286: 
> 1287:     void updateCSRLabel(List<Issue> issues) {

Why not private?

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

> 1287:     void updateCSRLabel(List<Issue> issues) {
> 1288:         if (issues.isEmpty()) {
> 1289:             log.info("No issue found for " + describe(pr));

I don't think this log message is relevant when this code is running as part of CheckRun.

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

> 1291:         }
> 1292: 
> 1293:         var versionOpt = BotUtils.getVersion(pr);

Can we reuse version from line 1167?

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

> 1316:             }
> 1317: 
> 1318:             var csrOptional = Backports.findCsr(jbsIssueOpt.get(), versionOpt.get());

Can this be unified with the `getCsrIssueTrackerIssues` call right before this method is called? We shouldn't need to fetch CSR issues more than once.

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

> 61:     private final boolean forceUpdate;
> 62: 
> 63:     CheckWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler, ZonedDateTime prUpdatedAt,

There are a lot of calls to the old constructor. Instead of adding `false` to all of them, perhaps overload the constructor with a default false.

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

> 276:         var activeReviews = CheckablePullRequest.filterActiveReviews(allReviews, pr.targetRef());
> 277:         // Determine if the current state of the PR has already been checked
> 278:         if (forceUpdate || !currentCheckValid(census, comments, activeReviews, labels)) {

Forcing an update is one way of solving this. It may turn out to trigger too many updates. If so we will need to think about how we can identify relevant changes to the issue, or at least filter out obviously irrelevant changes. We only really care about state changes.

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

> 100:                         .flatMap(r -> r.parsePullRequestUrl(uri.toString()).stream()))
> 101:                 .filter(Issue::isOpen)
> 102:                 .map(pr -> new CheckWorkItem(bot.getPRBot(pr.repository().name()), pr.id(), errorHandler, pr.updatedAt(), true, true))

The updatedAt parameter is used for the logLatency functionality. That means that the timestamp needs to be the timestamp of the action that caused this WorkItem to be scheduled. In this case that is the timestamp of the csrIssue update, not the pr. So this needs to be changed back to `csrIssue.updatedAt()` and the comment needs to be restored.

The rest of the code for logLatency in PR bot isn't aware of Issues now being able to trigger updates, so the variable names and log messages assume it's prUpdatedAt. This should ideally also be changed.

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

PR Review: https://git.openjdk.org/skara/pull/1492#pullrequestreview-1359953776
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149847408
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149848687
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149851405
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149851087
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149853017
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149862023
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149865068
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1149859255