RFR: 1588: Bridge messages should not be sent for PRs in draft state [v2]

Erik Joelsson erikj at openjdk.org
Mon Feb 13 19:18:14 UTC 2023

On Sat, 11 Feb 2023 12:19:48 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> This patch prevents the `mailling bridge bot` from sending emails when a PR is draft.
>> And the related test case is added.
>> Thanks for taking the time to review.
>> Best Regards,
>> -- Guoxiong
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>   Filter the comments in method ArchiveWorkItem#ignoreComment

This looks pretty good, but see comments. Were you able to run the manual tests to verify the new methods on PullRequest?

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java line 144:

> 142:         // after the last converted time should be ignored.
> 143:         if (pr.isDraft()) {
> 144:             var lastDraftTime = pr.lastMarkedAsDraftTime();

This method gets called for every comment on the PR, every time `ArchiveWorkItem` is run for a PR. The result of `pr.lastMarkedAsDraftTime()` needs to be retrieved once and reused.

forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java line 712:

> 710:                 .filter(obj -> obj.contains("event"))
> 711:                 .filter(obj -> obj.get("event").asString().equals("convert_to_draft"))
> 712:                 .reduce((a, b) -> b)

Are you sure of the order events are returned in? I couldn't find a definition of the order. Maybe safer to map to date and reduce to the highest one?

forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java line 792:

> 790:                 .filter(obj -> obj.get("system").asBoolean())
> 791:                 .filter(obj -> draftMessage.equals(obj.get("body").asString()))
> 792:                 .reduce((a, b) -> a)

Same here as for GitHub, maybe reduce to highest date just to be safe?


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

More information about the skara-dev mailing list