From efec17301293ba90f22e370148a5ec5b37b45ed2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 20 Mar 2026 19:54:15 +0000 Subject: [PATCH] Fix review comments: ConfigurationError wrapping, duplicate parse args, bool parsing, Kafka required topics, should_reload kwarg, SIGHUP test skips Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com> Agent-Logs-Url: https://github.com/domainaware/parsedmarc/sessions/0779003c-ccbe-4d76-9748-801dbc238b96 --- parsedmarc/__init__.py | 13 ++++++++----- parsedmarc/cli.py | 29 ++++++++++++++++++++--------- tests.py | 5 +++++ 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index a20501e..9d439b2 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -2249,11 +2249,14 @@ def watch_inbox( ) callback(res) - mailbox_connection.watch( - check_callback=check_callback, - check_timeout=check_timeout, - should_reload=should_reload, - ) + watch_kwargs: dict = { + "check_callback": check_callback, + "check_timeout": check_timeout, + } + if should_reload is not None: + watch_kwargs["should_reload"] = should_reload + + mailbox_connection.watch(**watch_kwargs) def append_json( diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py index 9c47292..9f001e2 100644 --- a/parsedmarc/cli.py +++ b/parsedmarc/cli.py @@ -201,8 +201,20 @@ def _parse_config_file(config_file, opts): "normalize_timespan_threshold_hours" ) if "index_prefix_domain_map" in general_config: - with open(general_config["index_prefix_domain_map"]) as f: - index_prefix_domain_map = yaml.safe_load(f) + map_path = general_config["index_prefix_domain_map"] + try: + with open(map_path) as f: + index_prefix_domain_map = yaml.safe_load(f) + except OSError as exc: + raise ConfigurationError( + "Failed to read index_prefix_domain_map file " + "'{0}': {1}".format(map_path, exc) + ) from exc + except yaml.YAMLError as exc: + raise ConfigurationError( + "Failed to parse YAML in index_prefix_domain_map " + "file '{0}': {1}".format(map_path, exc) + ) from exc if "offline" in general_config: opts.offline = bool(general_config.getboolean("offline")) if "strip_attachment_payloads" in general_config: @@ -588,9 +600,9 @@ def _parse_config_file(config_file, opts): "index setting missing from the splunk_hec config section" ) if "skip_certificate_verification" in hec_config: - opts.hec_skip_certificate_verification = hec_config[ - "skip_certificate_verification" - ] + opts.hec_skip_certificate_verification = bool( + hec_config.getboolean("skip_certificate_verification", fallback=False) + ) if "kafka" in config.sections(): kafka_config = config["kafka"] @@ -620,14 +632,14 @@ def _parse_config_file(config_file, opts): if "forensic_topic" in kafka_config: opts.kafka_forensic_topic = kafka_config["forensic_topic"] else: - logger.critical( + raise ConfigurationError( "forensic_topic setting missing from the kafka config section" ) if "smtp_tls_topic" in kafka_config: opts.kafka_smtp_tls_topic = kafka_config["smtp_tls_topic"] else: - logger.critical( - "forensic_topic setting missing from the splunk_hec config section" + raise ConfigurationError( + "smtp_tls_topic setting missing from the kafka config section" ) if "smtp" in config.sections(): @@ -1578,7 +1590,6 @@ def _main(): normalize_timespan_threshold_hours=24.0, fail_on_output_error=False, ) - args = arg_parser.parse_args() # Snapshot opts as set from CLI args / hardcoded defaults, before any config # file is applied. On each SIGHUP reload we restore this baseline first so diff --git a/tests.py b/tests.py index 64ac057..36389d2 100755 --- a/tests.py +++ b/tests.py @@ -4,6 +4,7 @@ from __future__ import absolute_import, print_function, unicode_literals import os +import signal import sys import tempfile import unittest @@ -1925,6 +1926,7 @@ password = pass watch = true """ + @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform") @patch("parsedmarc.cli._init_output_clients") @patch("parsedmarc.cli._parse_config_file") @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") @@ -1990,6 +1992,7 @@ watch = true # _parse_config_file called for initial load + reload self.assertGreaterEqual(mock_parse_config.call_count, 2) + @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform") @patch("parsedmarc.cli._init_output_clients") @patch("parsedmarc.cli._parse_config_file") @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") @@ -2062,6 +2065,7 @@ watch = true # The failed reload must not have closed the original clients initial_clients["s3_client"].close.assert_not_called() + @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform") @patch("parsedmarc.cli._init_output_clients") @patch("parsedmarc.cli._parse_config_file") @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") @@ -2133,6 +2137,7 @@ watch = true # Old client must have been closed when reload succeeded old_client.close.assert_called_once() + @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform") @patch("parsedmarc.cli._init_output_clients") @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") @patch("parsedmarc.cli.watch_inbox")