RFR: 1851: Fold CSR bot into PR bot [v3]
Erik Joelsson
erikj at openjdk.org
Tue Mar 28 22:18:41 UTC 2023
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
More information about the skara-dev
mailing list