From 90757adf263fb1f8301b886b12e26239b7f4b223 Mon Sep 17 00:00:00 2001 From: Ihor Solodrai Date: Tue, 7 Oct 2025 10:04:00 -0700 Subject: [PATCH 1/4] branch_worker: refactor send_email() 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 --- kernel_patches_daemon/branch_worker.py | 71 +++++++++++++++++--------- tests/test_branch_worker.py | 25 +++++++-- 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/kernel_patches_daemon/branch_worker.py b/kernel_patches_daemon/branch_worker.py index 2180143..f81eb65 100644 --- a/kernel_patches_daemon/branch_worker.py +++ b/kernel_patches_daemon/branch_worker.py @@ -21,7 +21,7 @@ from collections import namedtuple from contextlib import contextmanager from datetime import datetime, timedelta, timezone -from email.mime.application import MIMEApplication +from email.message import EmailMessage from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from enum import Enum @@ -248,6 +248,11 @@ def bump_email_status_counters(status: Status): def generate_msg_id(host: str) -> str: """Generate an email message ID based on the provided host.""" + # RFC 822 style message ID to allow for easier referencing of this + # message. Note that it's not entirely correct for us to refer to a host + # that is not entirely under our control, but we don't want to expose our + # actual host name either. Collisions of a sha256 hash are assumed to be + # unlikely in many contexts, so we do the same. checksum = hashlib.sha256(str(time.time()).encode("utf-8")).hexdigest() return f"{checksum}@{host}" @@ -265,11 +270,13 @@ def email_in_submitter_allowlist(email: str, allowlist: Sequence[re.Pattern]) -> def build_email( config: EmailConfig, - series: Series, + to_list: List[str], + cc_list: List[str], subject: str, msg_id: str, body: str, - boundary: str = "", + boundary: Optional[str] = None, + in_reply_to: Optional[str] = None, ) -> Tuple[List[str], str]: """ Builds complete email (including headers) to be sent along with curl command @@ -297,14 +304,6 @@ def build_email( "-", ] - to_list = copy.copy(config.smtp_to) - cc_list = copy.copy(config.smtp_cc) - - if config.ignore_allowlist or email_in_submitter_allowlist( - series.submitter_email, config.submitter_allowlist - ): - to_list += [series.submitter_email] - for to in to_list + cc_list: args += ["--mail-rcpt", to] @@ -312,13 +311,9 @@ def build_email( args += ["--proxy", config.smtp_http_proxy] msg = MIMEMultipart() - # Add some RFC 822 style message ID to allow for easier referencing of this - # message. Note that it's not entirely correct for us to refer to a host - # that is not entirely under our control, but we don't want to expose our - # actual host name either. Collisions of a sha256 hash are assumed to be - # unlikely in many contexts, so we do the same. msg["Message-Id"] = f"<{msg_id}>" - msg["In-Reply-To"] = get_ci_base(series)["msgid"] + if in_reply_to is not None: + msg["In-Reply-To"] = in_reply_to msg["References"] = msg["In-Reply-To"] msg["Subject"] = subject msg["From"] = config.smtp_from @@ -326,7 +321,7 @@ def build_email( msg["To"] = ",".join(to_list) if cc_list: msg["Cc"] = ",".join(cc_list) - if boundary: + if boundary is not None: msg.set_boundary(boundary) msg.attach(MIMEText(body, "plain")) @@ -335,14 +330,17 @@ def build_email( async def send_email( config: EmailConfig, - series: Series, + to_list: List[str], + cc_list: List[str], subject: str, body: str, + in_reply_to: Optional[str] = None, ): """Send an email.""" msg_id = generate_msg_id(config.smtp_host) - curl_args, msg = build_email(config, series, subject, msg_id, body) - + curl_args, msg = build_email( + config, to_list, cc_list, subject, msg_id, body, in_reply_to=in_reply_to + ) proc = await asyncio.create_subprocess_exec( *curl_args, stdin=PIPE, stdout=PIPE, stderr=PIPE ) @@ -353,6 +351,30 @@ async def send_email( email_send_fail_counter.add(1) +def ci_results_email_recipients( + config: EmailConfig, series: Series +) -> Tuple[List[str], List[str]]: + to_list = copy.copy(config.smtp_to) + cc_list = copy.copy(config.smtp_cc) + if config.ignore_allowlist or email_in_submitter_allowlist( + series.submitter_email, config.submitter_allowlist + ): + to_list += [series.submitter_email] + + return (to_list, cc_list) + + +async def send_ci_results_email( + config: EmailConfig, + series: Series, + subject: str, + body: str, +): + (to_list, cc_list) = ci_results_email_recipients(config, series) + in_reply_to = get_ci_base(series)["msgid"] + await send_email(config, to_list, cc_list, subject, body, in_reply_to) + + def pr_has_label(pr: PullRequest, label: str) -> bool: for pr_label in pr.get_labels(): if pr_label.name == label: @@ -559,7 +581,7 @@ def __init__( ) self.patchwork = patchwork - self.email = email + self.email_config = email self.log_extractor = log_extractor self.ci_repo_url = ci_repo_url @@ -1204,8 +1226,7 @@ async def evaluate_ci_result( self, status: Status, series: Series, pr: PullRequest, jobs: List[WorkflowJob] ) -> None: """Evaluate the result of a CI run and send an email as necessary.""" - email = self.email - if email is None: + if self.email_config is None: logger.info("No email configuration present; skipping sending...") return @@ -1245,7 +1266,7 @@ async def evaluate_ci_result( subject = await get_ci_email_subject(series) ctx = build_email_body_context(self.repo, pr, status, series, inline_logs) body = furnish_ci_email_body(ctx) - await send_email(email, series, subject, body) + await send_ci_results_email(self.email_config, series, subject, body) bump_email_status_counters(status) def expire_branches(self) -> None: diff --git a/tests/test_branch_worker.py b/tests/test_branch_worker.py index a5b2bac..583ea6a 100644 --- a/tests/test_branch_worker.py +++ b/tests/test_branch_worker.py @@ -29,10 +29,12 @@ BRANCH_TTL, BranchWorker, build_email, + ci_results_email_recipients, create_color_labels, email_in_submitter_allowlist, EmailBodyContext, furnish_ci_email_body, + get_ci_base, parse_pr_ref, prs_for_the_same_series, same_series_different_target, @@ -1316,13 +1318,17 @@ def test_email_submitter_in_allowlist(self): ] expected_email = read_fixture("test_email_submitter_in_allowlist.golden") + (to_list, cc_list) = ci_results_email_recipients(config, series) + in_reply_to = get_ci_base(series)["msgid"] cmd, email = build_email( config, - series, + to_list, + cc_list, "[PATCH bpf] my subject", "my-id", "body body body", boundary=self.BOUNDARY, + in_reply_to=in_reply_to, ) self.assertEqual(expected_cmd, cmd) self.assertEqual(expected_email, email) @@ -1371,8 +1377,17 @@ def test_email_submitter_not_in_allowlist(self): ] expected_email = read_fixture("test_email_submitter_not_in_allowlist.golden") + (to_list, cc_list) = ci_results_email_recipients(config, series) + in_reply_to = get_ci_base(series)["msgid"] cmd, email = build_email( - config, series, "my subject", "my-id", "body body", boundary=self.BOUNDARY + config, + to_list, + cc_list, + "my subject", + "my-id", + "body body", + boundary=self.BOUNDARY, + in_reply_to=in_reply_to, ) self.assertEqual(expected_cmd, cmd) self.assertEqual(expected_email, email) @@ -1425,13 +1440,17 @@ def test_email_submitter_not_in_allowlist_and_allowlist_disabled(self): "test_email_submitter_not_in_allowlist_and_allowlist_disabled.golden" ) + (to_list, cc_list) = ci_results_email_recipients(config, series) + in_reply_to = get_ci_base(series)["msgid"] cmd, email = build_email( config, - series, + to_list, + cc_list, "[PATCH bpf-next] some-subject", "my-id", "zzzzz\nzz", boundary=self.BOUNDARY, + in_reply_to=in_reply_to, ) self.assertEqual(expected_cmd, cmd) self.assertEqual(expected_email, email) From ccece32f436d9c352d7075100d06438ccecfbadd Mon Sep 17 00:00:00 2001 From: Ihor Solodrai Date: Wed, 8 Oct 2025 13:00:44 -0700 Subject: [PATCH 2/4] tests: factor out common/utils.py 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 --- tests/__init__.py | 10 ---- tests/common/patchwork_mock.py | 31 +------------ tests/common/utils.py | 46 +++++++++++++++++++ tests/{ => data}/fixtures/job_log_no_failures | 0 tests/{ => data}/fixtures/job_log_one_failure | 0 .../{ => data}/fixtures/job_log_two_failures | 0 tests/{ => data}/fixtures/kpd_config.json | 0 .../fixtures/test_email_body_conflict.golden | 0 .../fixtures/test_email_body_failure.golden | 0 .../fixtures/test_email_body_success.golden | 0 .../test_email_submitter_in_allowlist.golden | 0 ...st_email_submitter_not_in_allowlist.golden | 0 ...in_allowlist_and_allowlist_disabled.golden | 0 .../test_inline_email_text_multiple.golden | 0 .../test_inline_email_text_none.golden | 0 .../test_inline_email_text_single.golden | 0 tests/test_branch_worker.py | 2 +- tests/test_config.py | 15 +++--- tests/test_github_logs.py | 2 +- tests/test_github_sync.py | 3 +- tests/test_patchwork.py | 3 +- 21 files changed, 61 insertions(+), 51 deletions(-) create mode 100644 tests/common/utils.py rename tests/{ => data}/fixtures/job_log_no_failures (100%) rename tests/{ => data}/fixtures/job_log_one_failure (100%) rename tests/{ => data}/fixtures/job_log_two_failures (100%) rename tests/{ => data}/fixtures/kpd_config.json (100%) rename tests/{ => data}/fixtures/test_email_body_conflict.golden (100%) rename tests/{ => data}/fixtures/test_email_body_failure.golden (100%) rename tests/{ => data}/fixtures/test_email_body_success.golden (100%) rename tests/{ => data}/fixtures/test_email_submitter_in_allowlist.golden (100%) rename tests/{ => data}/fixtures/test_email_submitter_not_in_allowlist.golden (100%) rename tests/{ => data}/fixtures/test_email_submitter_not_in_allowlist_and_allowlist_disabled.golden (100%) rename tests/{ => data}/fixtures/test_inline_email_text_multiple.golden (100%) rename tests/{ => data}/fixtures/test_inline_email_text_none.golden (100%) rename tests/{ => data}/fixtures/test_inline_email_text_single.golden (100%) diff --git a/tests/__init__.py b/tests/__init__.py index eeb1aac..d9e785a 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -6,16 +6,6 @@ # pyre-strict -import importlib.resources import logging logging.disable() - - -def read_fixture(filepath: str) -> str: - return ( - importlib.resources.files(__package__) - .joinpath("fixtures") - .joinpath(filepath) - .read_text() - ) diff --git a/tests/common/patchwork_mock.py b/tests/common/patchwork_mock.py index 6889c7e..62e074b 100644 --- a/tests/common/patchwork_mock.py +++ b/tests/common/patchwork_mock.py @@ -6,11 +6,9 @@ # pyre-unsafe -import json -import os import re -from typing import Any, Dict, Final, List +from typing import Any, Dict, Final from aioresponses import aioresponses @@ -86,33 +84,6 @@ def get_dict_key(d: Dict[str, Any], idx: int = 0) -> str: return list(d)[idx] -def load_test_data_file(file): - with open(file, "r") as f: - if file.endswith(".json"): - return json.load(f) - else: - return f.read() - - -def load_test_data(path) -> Dict[str, Any]: - jsons_by_path = {} - with open(os.path.join(path, "responses.json"), "r") as f: - jsons_by_path = json.load(f) - data = {} - for url, value in jsons_by_path.items(): - match value: - case str(): - filepath = os.path.join(path, value) - data[url] = load_test_data_file(filepath) - case list(): - lst = [] - for filename in value: - filepath = os.path.join(path, filename) - lst.append(load_test_data_file(filepath)) - data[url] = lst - return data - - FOO_SERIES_FIRST = 2 FOO_SERIES_LAST = 10 diff --git a/tests/common/utils.py b/tests/common/utils.py new file mode 100644 index 0000000..fb802b2 --- /dev/null +++ b/tests/common/utils.py @@ -0,0 +1,46 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# All rights reserved. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +import json +import os +from typing import Any, Dict + + +def load_test_data_file(file): + with open(file, "r") as f: + if file.endswith(".json"): + return json.load(f) + else: + return f.read() + + +def load_test_data(path) -> Dict[str, Any]: + jsons_by_path = {} + with open(os.path.join(path, "responses.json"), "r") as f: + jsons_by_path = json.load(f) + data = {} + for url, value in jsons_by_path.items(): + match value: + case str(): + filepath = os.path.join(path, value) + data[url] = load_test_data_file(filepath) + case list(): + lst = [] + for filename in value: + filepath = os.path.join(path, filename) + lst.append(load_test_data_file(filepath)) + data[url] = lst + return data + + +def read_test_data_file(relative_path: str) -> str: + path = os.path.join(os.path.dirname(__file__), "../data", relative_path) + with open(path, "r") as f: + return f.read() + + +def read_fixture(filename: str) -> str: + return read_test_data_file(f"fixtures/{filename}") diff --git a/tests/fixtures/job_log_no_failures b/tests/data/fixtures/job_log_no_failures similarity index 100% rename from tests/fixtures/job_log_no_failures rename to tests/data/fixtures/job_log_no_failures diff --git a/tests/fixtures/job_log_one_failure b/tests/data/fixtures/job_log_one_failure similarity index 100% rename from tests/fixtures/job_log_one_failure rename to tests/data/fixtures/job_log_one_failure diff --git a/tests/fixtures/job_log_two_failures b/tests/data/fixtures/job_log_two_failures similarity index 100% rename from tests/fixtures/job_log_two_failures rename to tests/data/fixtures/job_log_two_failures diff --git a/tests/fixtures/kpd_config.json b/tests/data/fixtures/kpd_config.json similarity index 100% rename from tests/fixtures/kpd_config.json rename to tests/data/fixtures/kpd_config.json diff --git a/tests/fixtures/test_email_body_conflict.golden b/tests/data/fixtures/test_email_body_conflict.golden similarity index 100% rename from tests/fixtures/test_email_body_conflict.golden rename to tests/data/fixtures/test_email_body_conflict.golden diff --git a/tests/fixtures/test_email_body_failure.golden b/tests/data/fixtures/test_email_body_failure.golden similarity index 100% rename from tests/fixtures/test_email_body_failure.golden rename to tests/data/fixtures/test_email_body_failure.golden diff --git a/tests/fixtures/test_email_body_success.golden b/tests/data/fixtures/test_email_body_success.golden similarity index 100% rename from tests/fixtures/test_email_body_success.golden rename to tests/data/fixtures/test_email_body_success.golden diff --git a/tests/fixtures/test_email_submitter_in_allowlist.golden b/tests/data/fixtures/test_email_submitter_in_allowlist.golden similarity index 100% rename from tests/fixtures/test_email_submitter_in_allowlist.golden rename to tests/data/fixtures/test_email_submitter_in_allowlist.golden diff --git a/tests/fixtures/test_email_submitter_not_in_allowlist.golden b/tests/data/fixtures/test_email_submitter_not_in_allowlist.golden similarity index 100% rename from tests/fixtures/test_email_submitter_not_in_allowlist.golden rename to tests/data/fixtures/test_email_submitter_not_in_allowlist.golden diff --git a/tests/fixtures/test_email_submitter_not_in_allowlist_and_allowlist_disabled.golden b/tests/data/fixtures/test_email_submitter_not_in_allowlist_and_allowlist_disabled.golden similarity index 100% rename from tests/fixtures/test_email_submitter_not_in_allowlist_and_allowlist_disabled.golden rename to tests/data/fixtures/test_email_submitter_not_in_allowlist_and_allowlist_disabled.golden diff --git a/tests/fixtures/test_inline_email_text_multiple.golden b/tests/data/fixtures/test_inline_email_text_multiple.golden similarity index 100% rename from tests/fixtures/test_inline_email_text_multiple.golden rename to tests/data/fixtures/test_inline_email_text_multiple.golden diff --git a/tests/fixtures/test_inline_email_text_none.golden b/tests/data/fixtures/test_inline_email_text_none.golden similarity index 100% rename from tests/fixtures/test_inline_email_text_none.golden rename to tests/data/fixtures/test_inline_email_text_none.golden diff --git a/tests/fixtures/test_inline_email_text_single.golden b/tests/data/fixtures/test_inline_email_text_single.golden similarity index 100% rename from tests/fixtures/test_inline_email_text_single.golden rename to tests/data/fixtures/test_inline_email_text_single.golden diff --git a/tests/test_branch_worker.py b/tests/test_branch_worker.py index 583ea6a..7552da9 100644 --- a/tests/test_branch_worker.py +++ b/tests/test_branch_worker.py @@ -57,7 +57,7 @@ init_pw_responses, ) -from . import read_fixture +from tests.common.utils import load_test_data, read_fixture, read_test_data_file TEST_REPO = "repo" diff --git a/tests/test_config.py b/tests/test_config.py index 8d79fa3..63b8fce 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -7,7 +7,6 @@ # pyre-unsafe import json -import os import re import unittest from dataclasses import dataclass @@ -23,10 +22,12 @@ PatchworksConfig, ) +from tests.common.utils import read_fixture -def read_fixture(filepath: str) -> Dict[str, Union[str, int, bool, Dict]]: - with open(os.path.join(os.path.dirname(__file__), filepath)) as f: - return json.load(f) + +def load_kpd_config(filename: str) -> Dict[str, Union[str, int, bool, Dict]]: + raw_json = read_fixture(filename) + return json.loads(raw_json) class TestConfig(unittest.TestCase): @@ -38,7 +39,7 @@ def test_invalid_tag_to_branch_mapping(self) -> None: Tests that if a tag is mapped to a branch that doesn't exist, an exception is raised. """ # Load a valid config - kpd_config_json = read_fixture("fixtures/kpd_config.json") + kpd_config_json = load_kpd_config("kpd_config.json") @dataclass class TestCase: @@ -80,7 +81,7 @@ def test_valid_tag_to_branch_mapping(self) -> None: Tests combinaisons of valid tag_to_branch_mapping setup. """ # Load a valid config - kpd_config_json = read_fixture("fixtures/kpd_config.json") + kpd_config_json = load_kpd_config("kpd_config.json") @dataclass class TestCase: @@ -138,7 +139,7 @@ class TestCase: KPDConfig.from_json(conf) def test_valid(self) -> None: - kpd_config_json = read_fixture("fixtures/kpd_config.json") + kpd_config_json = load_kpd_config("kpd_config.json") with patch( "builtins.open", mock_open(read_data="TEST_KEY_FILE_CONTENT") diff --git a/tests/test_github_logs.py b/tests/test_github_logs.py index c84c101..c8e36f2 100644 --- a/tests/test_github_logs.py +++ b/tests/test_github_logs.py @@ -13,7 +13,7 @@ from kernel_patches_daemon.github_logs import BpfGithubLogExtractor, GithubFailedJobLog -from . import read_fixture +from tests.common.utils import read_fixture class MockWorkflowJob(WorkflowJob): diff --git a/tests/test_github_sync.py b/tests/test_github_sync.py index 6056bc6..818b588 100644 --- a/tests/test_github_sync.py +++ b/tests/test_github_sync.py @@ -21,7 +21,8 @@ ) from kernel_patches_daemon.config import KPDConfig, SERIES_TARGET_SEPARATOR from kernel_patches_daemon.github_sync import GithubSync -from tests.common.patchwork_mock import init_pw_responses, load_test_data, PatchworkMock +from tests.common.patchwork_mock import init_pw_responses, PatchworkMock +from tests.common.utils import load_test_data TEST_BRANCH = "test-branch" TEST_BPF_NEXT_BRANCH = "test-bpf-next" diff --git a/tests/test_patchwork.py b/tests/test_patchwork.py index 366351f..8ddff5e 100644 --- a/tests/test_patchwork.py +++ b/tests/test_patchwork.py @@ -32,10 +32,11 @@ get_default_pw_client, get_dict_key, init_pw_responses, - load_test_data, PatchworkMock, ) +from tests.common.utils import load_test_data + class PatchworkTestCase(unittest.IsolatedAsyncioTestCase): """ From f32c3e091a012a2e2972450ee4b409f14b4369ba Mon Sep 17 00:00:00 2001 From: Ihor Solodrai Date: Wed, 8 Oct 2025 13:10:28 -0700 Subject: [PATCH 3/4] branch_worker: rename email_in_submitter_allowlist Rename email_in_submitter_allowlist to email_matches_any to reflect its wider applicability. Signed-off-by: Ihor Solodrai --- kernel_patches_daemon/branch_worker.py | 13 +++---------- tests/test_branch_worker.py | 6 +++--- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/kernel_patches_daemon/branch_worker.py b/kernel_patches_daemon/branch_worker.py index f81eb65..1431a33 100644 --- a/kernel_patches_daemon/branch_worker.py +++ b/kernel_patches_daemon/branch_worker.py @@ -257,15 +257,8 @@ def generate_msg_id(host: str) -> str: return f"{checksum}@{host}" -def email_in_submitter_allowlist(email: str, allowlist: Sequence[re.Pattern]) -> bool: - """ - Checks if an email is in the submitter allowlist - - Note that there may be false positives when folks have regex syntax in - their email address. But that is ok -- this is simply a rollout mechanism. - We only need to roughly control the rollout. - """ - return any(regex.fullmatch(email) for regex in allowlist) +def email_matches_any(email: str, patterns: Sequence[re.Pattern]) -> bool: + return any(regex.fullmatch(email) for regex in patterns) def build_email( @@ -356,7 +349,7 @@ def ci_results_email_recipients( ) -> Tuple[List[str], List[str]]: to_list = copy.copy(config.smtp_to) cc_list = copy.copy(config.smtp_cc) - if config.ignore_allowlist or email_in_submitter_allowlist( + if config.ignore_allowlist or email_matches_any( series.submitter_email, config.submitter_allowlist ): to_list += [series.submitter_email] diff --git a/tests/test_branch_worker.py b/tests/test_branch_worker.py index 7552da9..1585967 100644 --- a/tests/test_branch_worker.py +++ b/tests/test_branch_worker.py @@ -31,7 +31,7 @@ build_email, ci_results_email_recipients, create_color_labels, - email_in_submitter_allowlist, + email_matches_any, EmailBodyContext, furnish_ci_email_body, get_ci_base, @@ -1249,7 +1249,7 @@ def test_allowlist_non_regex(self): for email, expected in cases: with self.subTest(msg=email): - result = email_in_submitter_allowlist(email, allowlist) + result = email_matches_any(email, allowlist) self.assertEqual(result, expected) def test_allowlist_regex_match(self): @@ -1270,7 +1270,7 @@ def test_allowlist_regex_match(self): for email, expected in cases: with self.subTest(msg=email): - result = email_in_submitter_allowlist(email, allowlist) + result = email_matches_any(email, allowlist) self.assertEqual(result, expected) def test_email_submitter_in_allowlist(self): From 3c76f0ec99f8cc4ddd2ff1c9fed95d230929d99d Mon Sep 17 00:00:00 2001 From: Ihor Solodrai Date: Wed, 8 Oct 2025 13:19:35 -0700 Subject: [PATCH 4/4] branch_worker: implement PR comment forwarding 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": ["bpf-ci-test@example.com"], "recipient_denylist": [".*@foobar.org"] } } Signed-off-by: Ihor Solodrai --- kernel_patches_daemon/branch_worker.py | 146 ++++++++++++++++++++++++- kernel_patches_daemon/config.py | 33 ++++++ kernel_patches_daemon/github_sync.py | 1 + kernel_patches_daemon/patchwork.py | 6 + tests/data/fixtures/kpd_config.json | 8 +- tests/test_branch_worker.py | 138 ++++++++++++++++++++++- tests/test_config.py | 8 ++ 7 files changed, 337 insertions(+), 3 deletions(-) diff --git a/kernel_patches_daemon/branch_worker.py b/kernel_patches_daemon/branch_worker.py index 1431a33..a8965ce 100644 --- a/kernel_patches_daemon/branch_worker.py +++ b/kernel_patches_daemon/branch_worker.py @@ -328,7 +328,7 @@ async def send_email( subject: str, body: str, in_reply_to: Optional[str] = None, -): +) -> str: """Send an email.""" msg_id = generate_msg_id(config.smtp_host) curl_args, msg = build_email( @@ -343,6 +343,8 @@ async def send_email( logger.error(f"failed to send email: {stdout.decode()} {stderr.decode()}") email_send_fail_counter.add(1) + return msg_id + def ci_results_email_recipients( config: EmailConfig, series: Series @@ -368,6 +370,74 @@ async def send_ci_results_email( await send_email(config, to_list, cc_list, subject, body, in_reply_to) +def reply_email_recipients( + msg: EmailMessage, + allowlist: Optional[Sequence[re.Pattern]] = None, + denylist: Optional[Sequence[re.Pattern]] = None, +) -> Tuple[List[str], List[str]]: + """ + Extracts response recipients from the `msg`, applying allowlist/denylist. + + Args: + msg: the EmailMessage we will be replying to + allowlist: list of email address regexes to allow, ignored if empty + denylist: list of email address regexes to deny, ignored if empty + + Returns: + (to_list, cc_list) - recipients of the reply email + """ + tos = msg.get_all("To", []) + ccs = msg.get_all("Cc", []) + cc_list = [a for (_, a) in email.utils.getaddresses(tos + ccs)] + + (_, sender_address) = email.utils.parseaddr(msg.get("From")) + to_list = [sender_address] + + if allowlist: + cc_list = [a for a in cc_list if email_matches_any(a, allowlist)] + to_list = [a for a in to_list if email_matches_any(a, allowlist)] + + if denylist: + cc_list = [a for a in cc_list if not email_matches_any(a, denylist)] + to_list = [a for a in to_list if not email_matches_any(a, denylist)] + + return (to_list, cc_list) + + +async def send_pr_comment_email( + config: EmailConfig, msg: EmailMessage, body: str +) -> Optional[str]: + """ + This function forwards a pull request comment (`body`) as an email reply to the original + patch email message `msg`. It extracts recipients from the `msg`, applies allowlist, denylist, + and always_cc as configured, and sets Subject and In-Reply-To based on the `msg` + + Args: + config: EmailConfig with PRCommentsForwardingConfig + msg: the original EmailMessage we are replying to (the patch submission) + body: the content of the reply we are sending + + Returns: + Message-Id of the sent email, or None if it wasn't sent + """ + if config is None or not config.is_pr_comment_forwarding_enabled(): + return + + cfg = config.pr_comments_forwarding + (to_list, cc_list) = reply_email_recipients( + msg, allowlist=cfg.recipient_allowlist, denylist=cfg.recipient_denylist + ) + cc_list += cfg.always_cc + + if not to_list and not cc_list: + return + + subject = "Re: " + msg.get("Subject") + in_reply_to = msg.get("Message-Id") + + return await send_email(config, to_list, cc_list, subject, body, in_reply_to) + + def pr_has_label(pr: PullRequest, label: str) -> bool: for pr_label in pr.get_labels(): if pr_label.name == label: @@ -1307,3 +1377,77 @@ async def submit_pr_summary( description="PR summary", ) pr_summary_report.add(1) + + async def forward_pr_comments(self, pr: PullRequest, series: Series): + if ( + self.email_config is None + or not self.email_config.is_pr_comment_forwarding_enabled() + ): + return + cfg = self.email_config.pr_comments_forwarding + + comments = pr.get_issue_comments() + + # Look for comments indicating what has already been forwarded + # to filter them out + forwarded_set = set() + for comment in comments: + match = re.search( + r"Forwarding comment \[([0-9]+)\].* via email", + comment.body, + re.MULTILINE, + ) + if not match: + continue + forwarded_id = int(match.group(1)) + forwarded_set.add(forwarded_id) + + for comment in comments: + if ( + comment.user.login not in cfg.commenter_allowlist + or comment.id in forwarded_set + ): + continue + # Determine the message to reply to + # Check for In-Reply-To-Subject tag in the comment body + match = re.search( + r"In-Reply-To-Subject: \`(.*)\`", comment.body, re.MULTILINE + ) + if not match: + logger.info( + f"Ignoring PR comment {comment.html_url} without In-Reply-To-Subject tag" + ) + continue + subject = match.group(1) + patch = series.patch_by_subject(subject) + if not patch: + logger.warn( + f"Ignoring PR comment {comment.html_url}, could not find relevant patch on patchwork" + ) + continue + + # first, post a comment that the message is being forwarded + msg_id = patch["msgid"] + patch_url = patch["web_url"] + message = f"Forwarding comment [{comment.id}]({comment.html_url}) via email" + message += f"\nIn-Reply-To: {msg_id}" + message += f"\nPatch: {patch_url}" + self._add_pull_request_comment(pr, message) + + # then, load the message we are replying to + mbox = await series.pw_client.get_blob(patch["mbox"]) + parser = email.parser.BytesParser(policy=email.policy.default) + patch_msg = parser.parsebytes(mbox, headersonly=True) + + # and forward the target comment via email + sent_msg_id = await send_pr_comment_email( + self.email_config, patch_msg, comment.body + ) + if sent_msg_id is not None: + logger.info( + f"Forwarded PR comment {comment.html_url} via email, Message-Id: {sent_msg_id}" + ) + else: + logger.warn( + f"Failed to forward PR comment in reply to {msg_id}, no recipients" + ) diff --git a/kernel_patches_daemon/config.py b/kernel_patches_daemon/config.py index b1f6bd6..b845f6e 100644 --- a/kernel_patches_daemon/config.py +++ b/kernel_patches_daemon/config.py @@ -104,6 +104,29 @@ def from_json(cls, json: Dict) -> "BranchConfig": ) +@dataclass +class PRCommentsForwardingConfig: + enabled: bool + always_cc: List[str] + commenter_allowlist: List[str] + recipient_allowlist: List[re.Pattern] + recipient_denylist: List[re.Pattern] + + @classmethod + def from_json(cls, json: Dict) -> "PRCommentsForwardingConfig": + return cls( + enabled=json.get("enabled", False), + always_cc=json.get("always_cc", []), + recipient_allowlist=[ + re.compile(pattern) for pattern in json.get("recipient_allowlist", []) + ], + recipient_denylist=[ + re.compile(pattern) for pattern in json.get("recipient_denylist", []) + ], + commenter_allowlist=json.get("commenter_allowlist", []), + ) + + @dataclass class EmailConfig: smtp_host: str @@ -123,6 +146,7 @@ class EmailConfig: # Ignore the `submitter_allowlist` entries and send emails to all patch # submitters, unconditionally. ignore_allowlist: bool + pr_comments_forwarding: Optional[PRCommentsForwardingConfig] @classmethod def from_json(cls, json: Dict) -> "EmailConfig": @@ -139,8 +163,17 @@ def from_json(cls, json: Dict) -> "EmailConfig": re.compile(pattern) for pattern in json.get("submitter_allowlist", []) ], ignore_allowlist=json.get("ignore_allowlist", False), + pr_comments_forwarding=PRCommentsForwardingConfig.from_json( + json.get("pr_comments_forwarding", {}) + ), ) + def is_pr_comment_forwarding_enabled(self) -> bool: + if self.pr_comments_forwarding is not None: + return self.pr_comments_forwarding.enabled + else: + return False + @dataclass class PatchworksConfig: diff --git a/kernel_patches_daemon/github_sync.py b/kernel_patches_daemon/github_sync.py index c179995..e2e216c 100644 --- a/kernel_patches_daemon/github_sync.py +++ b/kernel_patches_daemon/github_sync.py @@ -269,6 +269,7 @@ async def sync_relevant_subject(self, subject: Subject) -> None: f"Created/updated {pr} ({pr.head.ref}): {pr.url} for series {series.id}" ) await worker.sync_checks(pr, series) + await worker.forward_pr_comments(pr, series) # Close out other PRs if exists self.close_existing_prs_for_series(list(self.workers.values()), pr) diff --git a/kernel_patches_daemon/patchwork.py b/kernel_patches_daemon/patchwork.py index 662b6ae..6c962e8 100644 --- a/kernel_patches_daemon/patchwork.py +++ b/kernel_patches_daemon/patchwork.py @@ -497,6 +497,12 @@ def submitter_email(self): """Retrieve the email address of the patch series submitter.""" return self._submitter_email + def patch_by_subject(self, subject_str: str) -> Optional[Dict]: + for patch in self.patches: + if subject_str in patch["name"]: + return patch + return None + class Patchwork: def __init__( diff --git a/tests/data/fixtures/kpd_config.json b/tests/data/fixtures/kpd_config.json index b5ad95d..d05bf17 100644 --- a/tests/data/fixtures/kpd_config.json +++ b/tests/data/fixtures/kpd_config.json @@ -22,7 +22,13 @@ "pass": "super-secret-is-king", "http_proxy": "http://example.com:8080", "submitter_allowlist": ["email1-allow@example.com", "email2-allow@example.com"], - "ignore_allowlist": true + "ignore_allowlist": true, + "pr_comments_forwarding": { + "enabled": true, + "always_cc": ["bpf-ci-test@example.com"], + "commenter_allowlist": ["kpd-bot[bot]"], + "recipient_denylist": [".*@vger.kernel.org"] + } }, "tag_to_branch_mapping": { "tag": [ diff --git a/tests/test_branch_worker.py b/tests/test_branch_worker.py index 1585967..8cc2644 100644 --- a/tests/test_branch_worker.py +++ b/tests/test_branch_worker.py @@ -6,6 +6,9 @@ # pyre-unsafe +import email +import json +import os import random import re import shutil @@ -14,7 +17,7 @@ from dataclasses import dataclass, field from datetime import datetime from typing import Any, Dict, List, Optional -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import git @@ -37,12 +40,16 @@ get_ci_base, parse_pr_ref, prs_for_the_same_series, + reply_email_recipients, same_series_different_target, + send_pr_comment_email, temporary_patch_file, UPSTREAM_REMOTE_NAME, ) from kernel_patches_daemon.config import ( EmailConfig, + KPDConfig, + PRCommentsForwardingConfig, SERIES_ID_SEPARATOR, SERIES_TARGET_SEPARATOR, ) @@ -55,6 +62,7 @@ DEFAULT_TEST_RESPONSES, get_default_pw_client, init_pw_responses, + PatchworkMock, ) from tests.common.utils import load_test_data, read_fixture, read_test_data_file @@ -1289,6 +1297,7 @@ def test_email_submitter_in_allowlist(self): re.compile("a-user@example.com"), ], ignore_allowlist=False, + pr_comments_forwarding=None, ) expected_cmd = [ "curl", @@ -1350,6 +1359,7 @@ def test_email_submitter_not_in_allowlist(self): re.compile("email2-allow@example.com"), ], ignore_allowlist=False, + pr_comments_forwarding=None, ) expected_cmd = [ "curl", @@ -1409,6 +1419,7 @@ def test_email_submitter_not_in_allowlist_and_allowlist_disabled(self): re.compile("email2-allow@example.com"), ], ignore_allowlist=True, + pr_comments_forwarding=None, ) expected_cmd = [ "curl", @@ -1455,6 +1466,37 @@ def test_email_submitter_not_in_allowlist_and_allowlist_disabled(self): self.assertEqual(expected_cmd, cmd) self.assertEqual(expected_email, email) + def test_reply_email_recipients(self): + kpd_config_json = json.loads(read_fixture("kpd_config.json")) + kpd_config = KPDConfig.from_json(kpd_config_json) + self.assertIsNotNone(kpd_config) + + mbox = read_test_data_file( + "test_sync_patches_pr_summary_success/series-970926.mbox" + ) + parser = email.parser.BytesParser(policy=email.policy.default) + msg = parser.parsebytes(mbox.encode("utf-8"), headersonly=True) + self.assertIsNotNone(mbox) + denylist = kpd_config.email.pr_comments_forwarding.recipient_denylist + (to_list, cc_list) = reply_email_recipients(msg, denylist=denylist) + + self.assertEqual(to_list, ["chen.dylane@linux.dev"]) + self.assertEqual(len(cc_list), 17) + + # test allowlist by using the same denylist + (to_list, cc_list) = reply_email_recipients(msg, allowlist=denylist) + self.assertEqual(to_list, []) + self.assertEqual(len(cc_list), 3) + + # test both + allowlist = [re.compile(".*@linux.dev")] + denylist = [re.compile(".*@gmail.com")] + (to_list, cc_list) = reply_email_recipients( + msg, allowlist=allowlist, denylist=denylist + ) + self.assertEqual(to_list, ["chen.dylane@linux.dev"]) + self.assertEqual(len(cc_list), 3) + class TestParsePrRef(unittest.TestCase): def test_parse_pr_ref_comprehensive(self): @@ -1560,3 +1602,97 @@ def validate_parsed_pr_ref(ref): self.fail( f"parse_pr_ref raised {type(e).__name__} for input '{test_input}': {e}" ) + + +class TestForwardPrComments(unittest.IsolatedAsyncioTestCase): + def setUp(self) -> None: + patcher = patch("kernel_patches_daemon.github_connector.Github") + self._gh_mock = patcher.start() + self.addCleanup(patcher.stop) + + self._bw = BranchWorkerMock() + + async def asyncSetUp(self) -> None: + patchwork = PatchworkMock( + server="patchwork.test", + api_version="1.1", + search_patterns=[{"archived": False, "project": 399, "delegate": 121173}], + auth_token="mocktoken", + ) + self._pw = patchwork + + @aioresponses() + async def test_forward_pr_comments(self, m: aioresponses) -> None: + data_path = os.path.join( + os.path.dirname(__file__), + "data/test_sync_patches_pr_summary_success", + ) + test_data = load_test_data(data_path) + init_pw_responses(m, test_data) + + self._pw.get_blob = AsyncMock() + mbox = read_test_data_file( + "test_sync_patches_pr_summary_success/series-970926.mbox" + ) + self._pw.get_blob.return_value = mbox.encode("utf-8") + mbox_msgid = "20250611154859.259682-1-chen.dylane@linux.dev" + + self._bw.email_config = MagicMock( + pr_comments_forwarding=PRCommentsForwardingConfig( + enabled=True, + always_cc=["bpf-ci-test@example.com"], + commenter_allowlist=["test_user"], + recipient_denylist=[re.compile(".*@vger.kernel.org")], + recipient_allowlist=[], + ) + ) + + self._bw.patchwork = self._pw + series = await self._pw.get_series_by_id(970926) + + mock_comment = MagicMock() + mock_comment.id = 987654 + mock_comment.user.login = "test_user" + mock_comment.body = "Great work on this fix!\nIn-Reply-To-Subject: `bpf: clear user buf when bpf_d_path failed`" + mock_comment.html_url = ( + "https://github.com/example/repo/pull/42#issuecomment-987654" + ) + + mock_pr = MagicMock() + mock_pr.get_issue_comments.return_value = [mock_comment] + + with ( + patch.object(mock_pr, "create_issue_comment") as mock_create_comment, + patch("kernel_patches_daemon.branch_worker.send_email") as mock_send_email, + ): + mock_sent_msg = {"msgid": "forwarded-msg-id@example.com"} + mock_send_email.return_value = mock_sent_msg + + await self._bw.forward_pr_comments(mock_pr, series) + + mock_create_comment.assert_called_once() + posted_comment = mock_create_comment.call_args[0][0] + self.assertIn( + f"Forwarding comment [{mock_comment.id}]({mock_comment.html_url}) via email", + posted_comment, + ) + self.assertIn(f"In-Reply-To: <{mbox_msgid}>", posted_comment) + self.assertIn( + f"Patch: https://patchwork.test/project/netdevbpf/patch/{mbox_msgid}/", + posted_comment, + ) + + mock_send_email.assert_called_once() + (_, to_list, cc_list, subject, body, in_reply_to) = ( + mock_send_email.call_args[0] + ) + + self.assertEqual(to_list, ["chen.dylane@linux.dev"]) + self.assertEqual(len(cc_list), 18) + self.assertIn("bpf-ci-test@example.com", cc_list) + self.assertEqual( + "Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed", + subject, + ) + self.assertEqual(mock_comment.body, body) + self.assertEqual(f"<{mbox_msgid}>", in_reply_to) diff --git a/tests/test_config.py b/tests/test_config.py index 63b8fce..f2c0d53 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -20,6 +20,7 @@ InvalidConfig, KPDConfig, PatchworksConfig, + PRCommentsForwardingConfig, ) from tests.common.utils import read_fixture @@ -171,6 +172,13 @@ def test_valid(self) -> None: re.compile("email2-allow@example.com"), ], ignore_allowlist=True, + pr_comments_forwarding=PRCommentsForwardingConfig( + enabled=True, + always_cc=["bpf-ci-test@example.com"], + commenter_allowlist=["kpd-bot[bot]"], + recipient_denylist=[re.compile(".*@vger.kernel.org")], + recipient_allowlist=[], + ), ), tag_to_branch_mapping={"tag": ["app_auth_key_path"]}, branches={