RFR: 1851: Fold CSR bot into PR bot [v3]

[Erik Joelsson]

On Tue, 28 Mar 2023 19:47:18 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.
>
> Zhao Song has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - fix a problem
>  - fix a problem
>  - renamed some classes
>  - improve PullRequestBotFactoryTest

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

> 113:     }
> 114: 
> 115:     @Override

Should probably change the `workItemName` to something like "csrissue".

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestWorkItem.java line 47:

> 45:     /**
> 46:      * The updatedAt timestamp of the external entity that triggered this WorkItem,
> 47:      * which would be either a PR or a CSR Issue. Used for tracking reaction legacy

Suggestion:

     * which would be either a PR or a CSR Issue. Used for tracking reaction latency

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestWorkItem.java line 48:

> 46:      * The updatedAt timestamp of the external entity that triggered this WorkItem,
> 47:      * which would be either a PR or a CSR Issue. Used for tracking reaction legacy
> 48:      * of the bot through logging.This is the best estimated value, which is the last

Suggestion:

     * of the bot through logging. This is the best estimated value, which is the last

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestWorkItem.java line 51:

> 49:      * updatedAt value when the bot finds the PR or CSR Issue. This value is propagated
> 50:      * through chains of WorkItems, as the complete chain is considered to have
> 51:      * been triggered by the same PR update or CSR Issue update.

Suggestion:

     * been triggered by the same trigger.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestWorkItem.java line 62:

> 60:         this.prId = prId;
> 61:         this.errorHandler = errorHandler;
> 62:         this.triggerUpdatedAt = prUpdatedAt;

I like the new naming of this field, but please also update the parameter in the constructor.

bots/pr/src/test/java/org/openjdk/skara/bots/pr/CSRCommandTests.java line 453:

> 451: 
> 452:             // The bot should reply with a message that there is already an approved CSR request
> 453:             // Now CheckWorkItem is responsible for updating CSR label, so before '/csr' is handled, csr label is added to this pr

We should avoid comments that reference a change in the code. After this PR is integrated, it's no longer relevant that this functionality moved to CheckWorkItem.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1151195832
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1151200320
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1151200407
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1151200806
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1151199883
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1151202553