-
Notifications
You must be signed in to change notification settings - Fork 2
PR comments forwarding #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Change send_email() to decouple it from a Series object. send_email() will now unconditionally build and send an email message to provided recipients. The to/cc list filtering logic is moved to a new send_ci_results_email() function, which calls send_email(). Signed-off-by: Ihor Solodrai <[email protected]>
Consolidate test data loading code in common/utils.py Move `test/fixtures` to `test/data/fixtures` to simplify resource dependencies in BUCK. Signed-off-by: Ihor Solodrai <[email protected]>
Rename email_in_submitter_allowlist to email_matches_any to reflect its wider applicability. Signed-off-by: Ihor Solodrai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me. Left a few mostly cosmetic comments.
if not to_list: | ||
to_list = cfg.always_cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am a bit confused why we set Cc
addresses on as the To
. What's the thinking here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of the case when the patch author is filtered out. The filters shouldn't apply to always_cc
.
Do you know what happens if you try sending an email with curl with a cc
list but with empty to
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely recall using that for testing, so I think it should "just work". But probably best to try it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently the sending function concatenates both lists, so it should work:
for to in to_list + cc_list:
args += ["--mail-rcpt", to]
Thank you for the review. I'll push an update. |
Implement an optional generic PR comment forwarding feature. Whenever a relevant patchwork subject is processed in sync_relevant_subject(), inspect pull request comments and send selected comment body via email according to KPD configuration. This by design only affects pull request with corresponding up-to-date patch series on patchwork. To be forwarded the comment must pass the following filters: - the comment hasn't been forwarded yet - KPD is stateless, and so we determine this by checking for other comments on the same PR that report previous notifications - comment author's GitHub login is in configured allowlist - comment body contains an "In-Reply-To-Subject" tag with the subject string of a patch message to reply to - rationale for such tag is the necessity for a mechanism to determine the exact message in a thread to reply to; commit hash or index can change between series version, and a subject string uniquely identifies a patch message within a series When a comment is selected for forwarding KPD: - posts a PR comment reporting the forward action - sends an email to all original recipients of the patch message, filtered according to the configured allow/denylist An example of PR comments forwarding configuration: "email": { ... "pr_comments_forwarding": { "enabled": true, "commenter_allowlist": ["kpd-bot[bot]"], "always_cc": ["[email protected]"], "recipient_denylist": [".*@foobar.org"] } } Signed-off-by: Ihor Solodrai <[email protected]>
792bbbc
to
3c76f0e
Compare
LGTM, thanks. |
Motivation
KPD has an ability to send email notifications. Currently it is only used to report a CI run status to the patch submitter, and the email sending is pretty much hardcoded to do only that.
Recently automated AI reviews were implemented in BPF CI. The final output of such a review is an inline response to the patch email message, by LKML convention. This output is posted as a pull request comment (example). Of course AI reviews will be much more visible if they are sent as responses to corresponding patches on the mailing list.
By design KPD already has access to github, kernel patchwork and ability to send emails. So it is an obvious vehicle to implement such email responses. However it doesn't make sense to tightly couple "email responses" feature with AI review CI jobs specifically.
A more generic KPD feature that'll help to achieve the same is forwarding of PR comments via email.
Implementation
It is trivial for KPD to load PR comments content and put it into an email body. What is less trivial:
Filtering is achieved by an additional email sub-config with allowlist of github users. It also has to contain recipient filters, since the to/cc lists are different from CI status reports recipients.
To keep track of comments, github PR comments themselves can be used. When a comment is processed, KPD will post another comment informing what happened. This of course requires KPD-posted comment to have a particular format. For example:
Finally, to determine what mailing list message to respond to, we have to use some kind of a patch identifier within a series. It can't be a commit hash, because KPD regularly force-pushes changing them. It can't be a commit index in a series, because a new version of the series may change a position of particular patch.
A subject string is a reasonable identifier for this use-case. It's unique within a series. It corresponds to a specific commit and can be extracted from a commit. And of course it can be used to lookup an email Message-Id on patchwork, knowing the series.
This identifier then must be posted in comment body, so that KPD can find the relevant message. And it has to be done in a particular format as well, for example (mimicking email headers):
This is generic enough to be used for any kind of "forward PR comment as reply to a patch" use-case, as KPD relies only on its config and comments body and it's irrelevant how the comment was created.
Testing
In addition to unit tests, I manually tested the feature in BPF CI rc environment.
See example comments at kernel-patches/bpf-rc#6091