RFR: 2015: Initial run of PR IssueBot misses issue updates

[Erik Joelsson]

On Wed, 6 Sep 2023 18:19:19 GMT, Zhao Song <zsong at openjdk.org> wrote:

> Currently, if pullRequestBot is down for more than 10 minutes and during that time, if users make changes to some issues, than the update activities would not trigger CheckWorkItem after the bot restarts.
> 
> To solve this problem, In this patch, after the bot restarts, in the initial run, pullRequestBot would make CheckWorkItem compare both prMetaData and issueMetaData.

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

> 71: 
> 72:     CheckWorkItem(PullRequestBot bot, String prId, Consumer<RuntimeException> errorHandler, ZonedDateTime triggerUpdatedAt,
> 73:                   boolean needsReadyCheck, boolean forceUpdate, boolean spawnedFromIssueBot, boolean initialRun) {

Having 4 booleans in the constructor is becoming really hard to read and understand. Perhaps we should introduce factory methods instead so we only allow for valid combinations of the booleans. Something like:

fromIssue
fromPr
initialRun
forceUpdate

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

> 230:                         }
> 231:                     }
> 232:                     // triggered by pr updates

This comment should be moved out of the block.

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

> 230:                         if (previousIssueMetadata.equals(currIssueMetadata) && expiresAt.isAfter(Instant.now())) {
> 231:                             log.finer("[Issue]Metadata with expiration time is still valid, not checking again");
> 232:                             return true;

I think this method would be easier to read if we kept all return `true`/`false` inside the conditional blocks. If we know we have a terminal state, lets be clear about it so the reader doesn't need to check the rest of the method.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1554#discussion_r1323334134
PR Review Comment: https://git.openjdk.org/skara/pull/1554#discussion_r1323326749
PR Review Comment: https://git.openjdk.org/skara/pull/1554#discussion_r1323328980