1843: mlbridge bot bridges emails generated by itself #1485

lgxbslgx · 2023-03-16T05:20:54Z

Hi all,

This patch filters both github.com (by authenticatedUrl()) and git.openjdk.com (by url()) in MailingListArchiveReaderBot::inspect to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both pr.repository().url().getHost() and pr.repository().authenticatedUrl().getHost() always return null.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace

Issue

SKARA-1843: mlbridge bot bridges emails generated by itself

Reviewers

Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara.git pull/1485/head:pull/1485
$ git checkout pull/1485

Update a local copy of the PR:
$ git checkout pull/1485
$ git pull https://git.openjdk.org/skara.git pull/1485/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1485

View PR using the GUI difftool:
$ git pr show -t 1485

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1485.diff

bridgekeeper · 2023-03-16T05:20:56Z

👋 Welcome back gli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2023-03-16T05:24:17Z

Webrevs

lgxbslgx · 2023-03-16T10:33:27Z

In order to test this bug, I sent two emails [1][2] , but the bot didn't post them as comment. I think it may be another issue which is not related to this one. The current patch is still feasible.

[1] https://mail.openjdk.org/pipermail/skara-dev/2023-March/007597.html
[2] https://mail.openjdk.org/pipermail/skara-dev/2023-March/007600.html

mlbridge · 2023-03-16T13:22:58Z

Mailing list message from Guoxiong Li on skara-dev:

test mlbridge again
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/skara-dev/attachments/20230316/5aae3b54/attachment.htm>

lgxbslgx · 2023-03-16T13:46:33Z

Mailing list message from Guoxiong Li on skara-dev:

test mlbridge again -------------- next part -------------- An HTML attachment was scrubbed... URL: https://mail.openjdk.org/pipermail/skara-dev/attachments/20230316/5aae3b54/attachment.htm

This comment is posted successfully now. Is it done automatically? Or the bot is restarted, then this comment is posted?

erikj79 · 2023-03-16T15:27:31Z

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBot.java

+        var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";
+        var bridgeIdPattern = Pattern.compile(bridgeIdStr);


I think the correct fix is to just get the host from authenticatedUrl. Perhaps there should be a different method for this kind of usecase that better describes what we need here (the raw actual host, without the pattern rewrite), but at least for now, I think changing this to authenticatedUrl is a good enough fix. Thanks for figuring this out.

Just a reminder, If so, we should also change return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().url().getHost()) in ReviewArchive::getUniqueMessageId to return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().authenticatedUrl().getHost())

Yes, right, we are currently generating Message-IDs with the rewritten url. That's bad. We should make sure they are all reverted to github.com.

If we only filter the authenticatedUrl, what about the polluted data? The SKARA project now has many emails which have the message id with git.openjdk.org. If we don’t filter them, they would be posted as comments.

Yes, but it's only the Skara project, so I was ok with living with it. But sure, we can keep the double filter if we add the following comment.

Suggested change

var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";

var bridgeIdPattern = Pattern.compile(bridgeIdStr);

// Filter on both original, and any pattern replaced, host for now as we have polluted

// data in the Skara project mail archives. We should revert this to just authenticatedUrl

// eventually.

var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";

var bridgeIdPattern = Pattern.compile(bridgeIdStr);

Yes, but it's only the Skara project, so I was ok with living with it.

After thinking more, I am OK with it too. It can also verify and confirm my thought about the hosts in this patch.

Another option for us is that we use double filter right now and after 14 days, we revert it back to only filter authenticatedUrl(). In this way, the polluted Skara emails wouldn't be bridged.

Seems to only influence several PRs, so I don't want to complicate the flow.

Seems to only influence several PRs, so I don't want to complicate the flow.

Sure! I am also ok with it.

zhaosongzs · 2023-03-16T15:36:09Z

Mailing list message from Guoxiong Li on skara-dev:
test mlbridge again -------------- next part -------------- An HTML attachment was scrubbed... URL: https://mail.openjdk.org/pipermail/skara-dev/attachments/20230316/5aae3b54/attachment.htm

This comment is posted successfully now. Is it done automatically? Or the bot is restarted, then this comment is posted?

It is done automatically. It's a known issue. See SKARA-1644

zhaosongzs · 2023-03-16T15:41:29Z

I will check other places where we changed authenticatedUrl().getHost() to url().getHost() in SKARA-1813.

lgxbslgx · 2023-03-16T22:23:54Z

It is done automatically. It's a known issue. See SKARA-1644

Thanks. But the time is too long and unacceptable. Hope SKARA-1644 can be fixed.

openjdk · 2023-03-17T13:41:47Z

@lgxbslgx This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

After integration, the commit message for the final commit will be:

1843: mlbridge bot bridges emails generated by itself

Reviewed-by: erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

bead107: 1838: better error message when download fails

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

lgxbslgx · 2023-03-17T14:20:34Z

/integrate

openjdk · 2023-03-17T14:21:17Z

Going to push as commit 0853677.
Since your change was applied there has been 1 commit pushed to the master branch:

bead107: 1838: better error message when download fails

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-17T14:21:23Z

@lgxbslgx Pushed as commit 0853677.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mlbridge · 2023-03-17T14:45:08Z

Mailing list message from Guoxiong Li on skara-dev:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

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

Commit messages:
- SKARA-1843

Changes: https://git.openjdk.org/skara/pull/1485/files
Webrev: https://webrevs.openjdk.org/?repo=skara&pr=1485&range=00
Issue: https://bugs.openjdk.org/browse/SKARA-1843
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/skara/pull/1485.diff
Fetch: git fetch https://git.openjdk.org/skara.git pull/1485/head:pull/1485

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

mlbridge · 2023-03-17T14:45:12Z

Mailing list message from Guoxiong Li on skara-dev:

On Thu, 16 Mar 2023 05:20:54 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

In order to test this bug, I sent two emails [1][2] , but the bot didn't post them as comment. I think it may be another issue which is not related to this one. The current patch is still feasible.

[1] https://mail.openjdk.org/pipermail/skara-dev/2023-March/007597.html
[2] https://mail.openjdk.org/pipermail/skara-dev/2023-March/007600.html

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

PR Comment: https://git.openjdk.org/skara/pull/1485#issuecomment-1471696100

mlbridge · 2023-03-17T14:45:14Z

Mailing list message from Guoxiong Li on skara-dev:

On Thu, 16 Mar 2023 05:20:54 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

_Mailing list message from [Guoxiong Li](mailto:lgxbslgx at gmail.com) on [skara-dev](mailto:skara-dev at mail.openjdk.org):_

test mlbridge again -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/skara-dev/attachments/20230316/5aae3b54/attachment.htm>

This comment is posted successfully now. Is it done automatically? Or the bot is restarted, then this comment is posted?

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

PR Comment: https://git.openjdk.org/skara/pull/1485#issuecomment-1471990106

mlbridge · 2023-03-17T14:45:17Z

Mailing list message from Erik Joelsson on skara-dev:

On Thu, 16 Mar 2023 05:20:54 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBot.java line 118:

116: }
117: var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";
118: var bridgeIdPattern = Pattern.compile(bridgeIdStr);

I think the correct fix is to just get the host from `authenticatedUrl`. Perhaps there should be a different method for this kind of usecase that better describes what we need here (the raw actual host, without the pattern rewrite), but at least for now, I think changing this to `authenticatedUrl` is a good enough fix. Thanks for figuring this out.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1138882630

mlbridge · 2023-03-17T14:45:21Z

Mailing list message from Zhao Song on skara-dev:

On Thu, 16 Mar 2023 13:46:33 GMT, Guoxiong Li <gli at openjdk.org> wrote:

_Mailing list message from [Guoxiong Li](mailto:lgxbslgx at gmail.com) on [skara-dev](mailto:skara-dev at mail.openjdk.org):_
test mlbridge again -------------- next part -------------- An HTML attachment was scrubbed... URL: https://mail.openjdk.org/pipermail/skara-dev/attachments/20230316/5aae3b54/attachment.htm

This comment is posted successfully now. Is it done automatically? Or the bot is restarted, then this comment is posted?

It is done automatically. It's a known issue. See [SKARA-1644](https://bugs.openjdk.org/projects/SKARA/issues/SKARA-1644)

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

PR Comment: https://git.openjdk.org/skara/pull/1485#issuecomment-1472212637

mlbridge · 2023-03-17T14:45:23Z

Mailing list message from Zhao Song on skara-dev:

On Thu, 16 Mar 2023 05:20:54 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

I will check other places where we changed `authenticatedUrl().getHost()` to `url().getHost()` in SKARA-1813.

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

PR Comment: https://git.openjdk.org/skara/pull/1485#issuecomment-1472222138

mlbridge · 2023-03-17T14:45:25Z

Mailing list message from Zhao Song on skara-dev:

On Thu, 16 Mar 2023 15:27:31 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBot.java line 118:

116: }
117: var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";
118: var bridgeIdPattern = Pattern.compile(bridgeIdStr);

I think the correct fix is to just get the host from `authenticatedUrl`. Perhaps there should be a different method for this kind of usecase that better describes what we need here (the raw actual host, without the pattern rewrite), but at least for now, I think changing this to `authenticatedUrl` is a good enough fix. Thanks for figuring this out.

Just a reminder, If so, we should also change `return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().url().getHost())` in `ReviewArchive::getUniqueMessageId ` to `return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().authenticatedUrl().getHost())`

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1138918907

mlbridge · 2023-03-17T14:45:28Z

Mailing list message from Erik Joelsson on skara-dev:

On Thu, 16 Mar 2023 15:48:33 GMT, Zhao Song <zsong at openjdk.org> wrote:

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListArchiveReaderBot.java line 118:

116: }
117: var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";
118: var bridgeIdPattern = Pattern.compile(bridgeIdStr);

I think the correct fix is to just get the host from `authenticatedUrl`. Perhaps there should be a different method for this kind of usecase that better describes what we need here (the raw actual host, without the pattern rewrite), but at least for now, I think changing this to `authenticatedUrl` is a good enough fix. Thanks for figuring this out.

Just a reminder, If so, we should also change `return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().url().getHost())` in `ReviewArchive::getUniqueMessageId ` to `return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().authenticatedUrl().getHost())`

Yes, right, we are currently generating `Message-ID`s with the rewritten url. That's bad. We should make sure they are all reverted to github.com.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1139213720

mlbridge · 2023-03-17T14:45:31Z

Mailing list message from Guoxiong Li on skara-dev:

On Thu, 16 Mar 2023 18:34:15 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

Just a reminder, If so, we should also change `return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().url().getHost())` in `ReviewArchive::getUniqueMessageId ` to `return EmailAddress.from(encodedCommon + "." + UUID.randomUUID() + "@" + pr.repository().authenticatedUrl().getHost())`

Yes, right, we are currently generating `Message-ID`s with the rewritten url. That's bad. We should make sure they are all reverted to github.com.

If we only filter the `authenticatedUrl`, what about the polluted data? The SKARA project now has many emails which have the message id with `git.openjdk.org`. If we don?t filter them, they would be posted as comments.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1139411065

mlbridge · 2023-03-17T14:45:33Z

Mailing list message from Guoxiong Li on skara-dev:

On Thu, 16 Mar 2023 15:36:09 GMT, Zhao Song <zsong at openjdk.org> wrote:

It is done automatically. It's a known issue. See [SKARA-1644](https://bugs.openjdk.org/projects/SKARA/issues/SKARA-1644)

Thanks. But the time is too long and unacceptable. Hope SKARA-1644 can be fixed.

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

PR Comment: https://git.openjdk.org/skara/pull/1485#issuecomment-1472836645

mlbridge · 2023-03-17T14:45:36Z

Mailing list message from Guoxiong Li on skara-dev:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

Change 'url' to 'authenticatedUrl'

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

Changes:
- all: https://git.openjdk.org/skara/pull/1485/files
- new: https://git.openjdk.org/skara/pull/1485/files/b9fa0ec7..01cba34

Webrevs:
- full: https://webrevs.openjdk.org/?repo=skara&pr=1485&range=01
- incr: https://webrevs.openjdk.org/?repo=skara&pr=1485&range=00-01

Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/skara/pull/1485.diff
Fetch: git fetch https://git.openjdk.org/skara.git pull/1485/head:pull/1485

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

mlbridge · 2023-03-18T04:53:47Z

Mailing list message from Erik Joelsson on skara-dev:

On Thu, 16 Mar 2023 22:16:44 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Yes, right, we are currently generating `Message-ID`s with the rewritten url. That's bad. We should make sure they are all reverted to github.com.

If we only filter the `authenticatedUrl`, what about the polluted data? The SKARA project now has many emails which have the message id with `git.openjdk.org`. If we don?t filter them, they would be posted as comments.

Yes, but it's only the Skara project, so I was ok with living with it. But sure, we can keep the double filter if we add the following comment.
Suggestion:

// Filter on both original, and any pattern replaced, host for now as we have polluted
// data in the Skara project mail archives. We should revert this to just authenticatedUrl
// eventually.
var bridgeIdStr = "^[^.]+\.[^.]+@(?:" + pr.repository().url().getHost() + "|" + pr.repository().authenticatedUrl().getHost() + ")$";
var bridgeIdPattern = Pattern.compile(bridgeIdStr);

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1140199035

mlbridge · 2023-03-18T04:53:49Z

Mailing list message from Guoxiong Li on skara-dev:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

Use the simple pattern.

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

Changes:
- all: https://git.openjdk.org/skara/pull/1485/files
- new: https://git.openjdk.org/skara/pull/1485/files/01cba348..8ff1e0f

Webrevs:
- full: https://webrevs.openjdk.org/?repo=skara&pr=1485&range=02
- incr: https://webrevs.openjdk.org/?repo=skara&pr=1485&range=01-02

Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
Patch: https://git.openjdk.org/skara/pull/1485.diff
Fetch: git fetch https://git.openjdk.org/skara.git pull/1485/head:pull/1485

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

mlbridge · 2023-03-18T04:53:50Z

Mailing list message from Guoxiong Li on skara-dev:

On Fri, 17 Mar 2023 12:53:12 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

Yes, but it's only the Skara project, so I was ok with living with it.

After thinking more, I am OK with it too. It can also verify and confirm my thought about the hosts in this patch.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1140228349

mlbridge · 2023-03-18T04:53:51Z

Mailing list message from Erik Joelsson on skara-dev:

On Fri, 17 Mar 2023 13:25:36 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

Use the simple pattern.

Marked as reviewed by erikj (Lead).

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

PR Review: https://git.openjdk.org/skara/pull/1485#pullrequestreview-1345974226

mlbridge · 2023-03-18T04:53:52Z

Mailing list message from Zhao Song on skara-dev:

On Fri, 17 Mar 2023 13:19:47 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Yes, but it's only the Skara project, so I was ok with living with it. But sure, we can keep the double filter if we add the following comment.
Suggestion:
    \/\/ Filter on both original\, and any pattern replaced\, host for now as we have polluted
    \/\/ data in the Skara project mail archives\. We should revert this to just authenticatedUrl
    \/\/ eventually\.
    var bridgeIdStr \= \"\^\[\^\.\]\+\\\.\[\^\.\]\+\@$\?\:\" \+ pr\.repository\($\.url\.getHost \+ \"\|\" \+ pr\.repository\.authenticatedUrl\.getHost \+ \"\)\$\"\;
    var bridgeIdPattern \= Pattern\.compile$bridgeIdStr$\;
Yes, but it's only the Skara project, so I was ok with living with it.

After thinking more, I am OK with it too. It can also verify and confirm my thought about the hosts in this patch.

Another option for us is that we use double filter right now and after 14 days, we revert it back to only filter `authenticatedUrl()`. In this way, the polluted Skara emails wouldn't be bridged.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1140254977

mlbridge · 2023-03-18T04:53:53Z

Mailing list message from Guoxiong Li on skara-dev:

On Fri, 17 Mar 2023 13:42:27 GMT, Zhao Song <zsong at openjdk.org> wrote:

Yes, but it's only the Skara project, so I was ok with living with it.

After thinking more, I am OK with it too. It can also verify and confirm my thought about the hosts in this patch.

Another option for us is that we use double filter right now and after 14 days, we revert it back to only filter `authenticatedUrl()`. In this way, the polluted Skara emails wouldn't be bridged.

Seems to only influence several PRs, so I don't want to complicate the flow.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1140271614

mlbridge · 2023-03-18T04:53:54Z

Mailing list message from Zhao Song on skara-dev:

On Fri, 17 Mar 2023 13:56:44 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Seems to only influence several PRs, so I don't want to complicate the flow.

Sure! I am also ok with it.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1485#discussion_r1140272665

mlbridge · 2023-03-18T04:53:55Z

Mailing list message from Guoxiong Li on skara-dev:

On Thu, 16 Mar 2023 05:20:54 GMT, Guoxiong Li <gli at openjdk.org> wrote:

Hi all,

This patch filters both `github.com` (by `authenticatedUrl()`) and `git.openjdk.com` (by `url()`) in `MailingListArchiveReaderBot::inspect` to avoid the bot posting the unnecessary comments to the PRs. Please see SKARA-1843 [1] for more information.

But I can't write a test case for it now, because the current URLs of the repositories in test code are fake and their host are empty, which means both `pr.repository().url().getHost()` and `pr.repository().authenticatedUrl().getHost()` always return `null`.

Thanks for the review.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.org/browse/SKARA-1843

This pull request has now been integrated.

Changeset: 0853677
Author: Guoxiong Li <gli at openjdk.org>
URL: https://git.openjdk.org/skara/commit/0853677abeb6d22580b3f4d0b0bfe03b4736630e
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod

1843: mlbridge bot bridges emails generated by itself

Reviewed-by: erikj

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

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

SKARA-1843

b9fa0ec

openjdk bot changed the title ~~SKARA-1843~~ 1843: mlbridge bot bridge emails generated by itself. Mar 16, 2023

openjdk bot added the rfr label Mar 16, 2023

erikj79 reviewed Mar 16, 2023

View reviewed changes

Change 'url' to 'authenticatedUrl'

01cba34

lgxbslgx changed the title ~~1843: mlbridge bot bridge emails generated by itself.~~ 1843: mlbridge bot bridges emails generated by itself Mar 16, 2023

Use the simple pattern.

8ff1e0f

erikj79 approved these changes Mar 17, 2023

View reviewed changes

openjdk bot added the ready label Mar 17, 2023

openjdk bot added the integrated label Mar 17, 2023

openjdk bot closed this Mar 17, 2023

openjdk bot removed ready rfr labels Mar 17, 2023

lgxbslgx deleted the SKARA-1843 branch March 17, 2023 14:23

		var bridgeIdStr = "^[^.]+\\.[^.]+@(?:" + pr.repository().url().getHost() + "\|" + pr.repository().authenticatedUrl().getHost() + ")$";
		var bridgeIdPattern = Pattern.compile(bridgeIdStr);

1843: mlbridge bot bridges emails generated by itself #1485

1843: mlbridge bot bridges emails generated by itself #1485

Conversation

lgxbslgx commented Mar 16, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 16, 2023

mlbridge bot commented Mar 16, 2023 • edited

Webrevs

lgxbslgx commented Mar 16, 2023

mlbridge bot commented Mar 16, 2023

lgxbslgx commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikj79 Mar 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaosongzs commented Mar 16, 2023

zhaosongzs commented Mar 16, 2023

lgxbslgx commented Mar 16, 2023

openjdk bot commented Mar 17, 2023

lgxbslgx commented Mar 17, 2023

openjdk bot commented Mar 17, 2023

openjdk bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 17, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

mlbridge bot commented Mar 18, 2023

lgxbslgx commented Mar 16, 2023 •

edited by openjdk bot

mlbridge bot commented Mar 16, 2023 •

edited

erikj79 Mar 16, 2023 •

edited