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

Erik Joelsson erikj at openjdk.org
Tue Mar 28 19:47:19 UTC 2023


On Tue, 28 Mar 2023 00:58:31 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> 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.
>
> Yes, you're right. Currently, any change to a csr issue would trigger update to the relevant pr. I will try to get the activities of the csr issue.

I think it's better to leave that for later. If we see that this becomes a problem, we could think about possible solutions. Let's try to keep the scope of this change from growing too much.

>> bots/pr/src/test/java/org/openjdk/skara/bots/pr/PullRequestBotFactoryTest.java line 173:
>> 
>>> 171:             assertNotNull(csrIssueBot2.getPRBot("repo2"));
>>> 172:             assertNotNull(csrIssueBot2.getPRBot("repo5"));
>>> 173:             assertNotNull(csrIssueBot2.getPRBot("repo6"));
>> 
>> This doesn't look right. An instance of `CSRIssueBot` should only have access to `PullRequestBot` instances for repositories that share the same IssueProject.
>
> At first, I had the same idea with you and later I found it a little complex to implement it. But if we let all CSRIssueBots share a map, it would be very easy. If you think it's necessary to change, I will fix it.

I suppose it doesn't matter if they share the map, but the test does not need to verify that they do, or that a `CSRIssueBot` for a certain `IssueProject` is able to look up a `PullRequestBot` for a different IssueProject. The test should only verify that the relevant PullRequestBots are available.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1150877603
PR Review Comment: https://git.openjdk.org/skara/pull/1492#discussion_r1150835008


More information about the skara-dev mailing list