RFR: 1789: Skara PR commands are not interpreted on Github review comments [v2]

Kevin Rushforth kcr at openjdk.org
Thu Jan 19 16:01:15 UTC 2023

On Wed, 18 Jan 2023 21:41:03 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> A user reported that Skara bots were not responding to commands in review comments or the body of a review. 
>> This patch addresses that issue by ensuring that the Skara bots can correctly detect and interpret commands.
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
>   sort comments by created time

Since there is general agreement not to attempt to use inline "file" comments, I left a couple suggestions to remove some commented out code.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java line 233:

> 231: 
> 232:     /**
> 233:      * This method returns all the comments in the pr including comments in reviews(review body) and file specific comments

Given the discussion around only using "ordinary" PR comments, and not inline comments, I think the "and file specific comments" part is now wrong.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCommandWorkItem.java line 245:

> 243: //        comments.addAll(pr.reviewComments().stream()
> 244: //                .map(reviewComment -> new Comment("ReviewComment" + reviewComment.id(), reviewComment.body(), reviewComment.author(), reviewComment.createdAt(), reviewComment.updatedAt()))
> 245: //                .toList());

Since we have decided not to do this, I recommend to remove this commented out code.

bots/pr/src/test/java/org/openjdk/skara/bots/pr/PullRequestCommandTests.java line 395:

> 393: //            // Run the bot
> 394: //            TestBotRunner.runPeriodicItems(prBot);
> 395: //            assertLastCommentContains(pr, "The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 4");

Ditto the recommendation to remove this.


PR: https://git.openjdk.org/skara/pull/1458

More information about the skara-dev mailing list