#54 - Add QA tooling #55
Conversation
miroslavpojer
requested review from
ABMC831,
OlivieFranklova,
Zejnilovic,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
oto-macenauer-absa
force-pushed
the
feature/54-Add-QA-tooling
branch
from
b72beaf to
caa9bc2
Compare
WalkthroughAdds QA tooling (CI workflow, pylint, mypy, Black, pytest), developer docs, pyproject and top-level tooling requirements; updates CODEOWNERS and .gitignore; refactors Lambda and writer modules with typed init/write APIs, UTF‑8 config loading, JWT handling, and aggregated writer errors; migrates and expands tests to pytest; empties Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as API Gateway
participant L as Lambda (event_gate_lambda)
participant TP as Token Provider
participant Wk as Writer: Kafka
participant We as Writer: EventBridge
participant Wp as Writer: Postgres
C->>API: POST /topics/{topic}\nAuthorization: Bearer <JWT>\nJSON body
API->>L: invoke
rect rgba(230,245,255,0.35)
note over L: Cold start / init
L->>L: load CONF_DIR, ACCESS, TOPICS
L->>TP: GET public key (RS256, timeout)
TP-->>L: public key
end
L->>L: extract_token -> decode & verify JWT
alt topic missing
L-->>API: 404 {errors: [...]}
else unauthorized
L-->>API: 403 {errors: [...]}
else schema invalid
L-->>API: 400 {errors: [...]}
else valid
par dispatch to writers
L->>Wk: write(topic, message)
L->>We: write(topic, message)
L->>Wp: write(topic, message)
end
alt any writer failed
L-->>API: 500 {errors: [writer errors...]}
else all succeeded or skipped
L-->>API: 202 {success:true}
end
end
API-->>C: HTTP response (JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/writer_kafka.py (2)
1-6: Define globals for mypy, drop unused import, and improve typingRemoves unused boto3, adds logging, and pre-declares globals to satisfy mypy (pipeline complained about undefined symbols).
Apply:
-import json - -import boto3 -from confluent_kafka import Producer +import json +import logging +from typing import Any, Optional +from confluent_kafka import Producer # type: ignore[import-untyped] + +# Initialized in init() +_logger: Optional[logging.Logger] = None +kafka_producer: Any = None # Producer | None, but Producer lacks stubs
1-51: Remove tuple unpacking from write() call sites (now returns bool)
- src/event_gate_lambda.py:144 change
kafka_ok, kafka_err = writer_kafka.write(topicName, topicMessage)tokafka_ok = writer_kafka.write(topicName, topicMessage)- src/event_gate_lambda.py:145 change
eventbridge_ok, eventbridge_err = writer_eventbridge.write(topicName, topicMessage)toeventbridge_ok = writer_eventbridge.write(topicName, topicMessage)- src/event_gate_lambda.py:146 change
postgres_ok, postgres_err = writer_postgres.write(topicName, topicMessage)topostgres_ok = writer_postgres.write(topicName, topicMessage)
Handle error logging inside the writer or introduce a separate mechanism if message details are still required.src/writer_eventbridge.py (1)
1-4: Mypy: declare globals with types and import logging; initialize to safe defaults.Prevents “undefined _logger/EVENT_BUS_ARN/aws_eventbridge” and helps tooling.
Apply:
import json +import logging +from typing import Optional, Any - import boto3 +try: + from botocore.client import BaseClient +except Exception: + BaseClient = Any # fallback for type checking + +_logger: logging.Logger +EVENT_BUS_ARN: str = "" +aws_eventbridge: Optional["BaseClient"] = None @@ -def init(logger, CONFIG): +def init(logger: logging.Logger, CONFIG: dict) -> None: @@ - aws_eventbridge = boto3.client("events") + aws_eventbridge = boto3.client("events")Also applies to: 6-15
src/event_gate_lambda.py (3)
48-76: Avoid network/file I/O at import time (cold-start fragility).Lazy-load ACCESS/TOKEN_PUBLIC_KEY on first use and cache.
If you want a minimal change now, keep as-is; otherwise I can provide a follow-up patch to load/cached in getters (ACCESS(), TOKEN_PUBLIC_KEY()) and handle retries.
Also applies to: 78-81
201-204: Use logger.exception for unhandled errors in the dispatcher.Improves diagnostics and satisfies linters.
- except Exception as e: - logger.error(f"Unexpected exception: {e}") + except Exception: + logger.exception("Unexpected exception") return _error_response(500, "internal", "Unexpected server error")
143-167: Remove tuple unpacking from writer.write() calls — write() now returns only a boolean.Replace in src/event_gate_lambda.py at lines 144–146 and adjust error messages:
- kafka_ok, kafka_err = writer_kafka.write(topicName, topicMessage) - eventbridge_ok, eventbridge_err = writer_eventbridge.write(topicName, topicMessage) - postgres_ok, postgres_err = writer_postgres.write(topicName, topicMessage) + kafka_ok = writer_kafka.write(topicName, topicMessage) + eventbridge_ok = writer_eventbridge.write(topicName, topicMessage) + postgres_ok = writer_postgres.write(topicName, topicMessage) - if not kafka_ok: - errors.append({"type": "kafka", "message": kafka_err}) - if not eventbridge_ok: - errors.append({"type": "eventbridge", "message": eventbridge_err}) - if not postgres_ok: - errors.append({"type": "postgres", "message": postgres_err}) + if not kafka_ok: + errors.append({"type": "kafka", "message": "write failed"}) + if not eventbridge_ok: + errors.append({"type": "eventbridge", "message": "write failed"}) + if not postgres_ok: + errors.append({"type": "postgres", "message": "write failed"})
🧹 Nitpick comments (23)
.gitignore (1)
9-9: Add ignores for QA/cache artifacts and common local envs.Since this PR adds QA tooling, ignore their caches and common local dirs to keep the repo clean.
/.idea/ + +# Local IDE and venvs +/.vscode/ +venv/ + +# QA/tooling caches & reports +.pytest_cache/ +.mypy_cache/ +.pylint.d/ +.coverage +htmlcov/pyproject.toml (1)
11-11: Tighten mypy exclude to just the tests directory.Plain "tests" matches any path containing the substring. Use an anchored regex.
-exclude = "tests" +exclude = "^tests/.*$"requirements.txt (2)
15-15: Consider psycopg2 vs psycopg2-binary.Binary wheel is convenient for dev/CI, but upstream advises source build for production. If this list is used for Lambda packaging, confirm the choice.
1-15: Split runtime vs dev dependencies.Mixing prod libs (boto3, confluent_kafka, psycopg2-binary) with tooling (pytest, pylint, black, mypy) makes Lambda layers/containers heavier. Suggest requirements.txt (runtime) + requirements-dev.txt (tooling) or extras in pyproject.
DEVELOPER.md (5)
3-10: Remove TOC entry without a section, or add the section.“Run Action Locally” has no heading below, triggering MD051.
- [Run Action Locally](#run-action-locally)
71-74: Specify language on the fenced output block.Satisfy MD040; use text for sample outputs.
-``` +```text All done! ✨ 🍰 ✨ 1 file reformatted.--- `20-25`: **Align venv name with .gitignore.** Docs use “venv” but .gitignore excludes “.venv”. Either switch to .venv in docs or add venv/ to .gitignore. --- `127-128`: **Remove stray empty heading at the end.** ```diff -## -
121-125: Make coverage report open command OS-agnostic.Use Python’s webbrowser for portability.
-open htmlcov/index.html +python -c "import webbrowser; webbrowser.open('htmlcov/index.html')".pylintrc (2)
41-43: Align fail-under with team target (9.5) or drop it.Docs say aim ≥9.5; config enforces 10, which will fail locally even when CI passes.
-fail-under=10 +# Keep in sync with CI target (see workflow). Consider removing and letting CI enforce. +fail-under=9.5
121-122: Consider linting tests too.Excluding tests reduces signal on fixtures and helper modules. Optional, but recommended over time.
-ignore-paths=tests +# ignore-paths=teststests/conftest.py (2)
1-1: Split multiple imports onto separate linesAvoid
import os, sys; some linters flag C0410.Apply:
-import os, sys +import os +import sys
3-5: Path bootstrapping is fine; consider removing once CI installs the packageThis sys.path tweak is acceptable short-term, but ideally the workflow should
pip install -e .(or fix PYTHONPATH there) so tests don’t need to mutate sys.path..github/workflows/test.yml (2)
47-56: Unify action versions for consistencyUse the same major tags across jobs to avoid subtle differences.
Apply:
- uses: actions/checkout@v4.1.5 + uses: actions/checkout@v4 ... - uses: actions/setup-python@v5.1.0 + uses: actions/setup-python@v5Repeat the same normalization in the mypy job (Lines 101-109).
28-41: Harden Pylint score extractionPiping through grep/awk can break on unexpected output. Make pylint non-fatal for parsing, then gate separately.
Apply:
- pylint_score=$(pylint $(git ls-files '*.py')| grep 'rated at' | awk '{print $7}' | cut -d'/' -f1) + set -o pipefail + pylint $(git ls-files '*.py') --exit-zero | tee pylint.out + pylint_score=$(grep 'rated at' pylint.out | tail -n1 | awk '{print $7}' | cut -d'/' -f1)src/writer_kafka.py (3)
7-13: Guard against use before init and add minimal type hintsMake it explicit that init must be called first; helps static checkers and runtime safety.
Apply:
-def init(logger, CONFIG): +def init(logger: "logging.Logger", CONFIG: dict) -> None: global _logger global kafka_producer @@ - _logger = logger + _logger = loggerAnd consider asserting required config keys (see next comment).
33-51: Ensure init() was called before write()If write is called before init,
_logger/kafka_producermay be None.Apply:
def write(topicName, message): - try: + try: + if _logger is None or kafka_producer is None: + raise RuntimeError("Kafka writer not initialized; call init() first")
1-4: Optional: lazy-import confluent_kafka to make Kafka optional at runtimeIf Kafka isn’t installed in all environments, defer import to init and fail gracefully.
If desired:
# at module top try: from confluent_kafka import Producer # type: ignore[import-untyped] except ImportError: Producer = None # type: ignore[assignment]Then in init():
if Producer is None: raise RuntimeError("confluent_kafka is not installed; Kafka writer unavailable")src/writer_eventbridge.py (1)
18-21: Guard against uninitialized client; treat missing config as “skipped” with a warning.Small safety for cases where init wasn’t called.
def write(topicName, message): + if aws_eventbridge is None: + _logger.error("EventBridge client not initialized") + return False if not EVENT_BUS_ARN: - _logger.debug("No EventBus Arn - skipping") + _logger.warning("No EventBus Arn - skipping EventBridge write") return Truesrc/writer_postgres.py (2)
22-29: Handle Secrets Manager failures explicitly (optional).If the secret is missing/denied, log and fall back gracefully.
- if secret_name and secret_region: - aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) - postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] - POSTGRES = json.loads(postgres_secret) + if secret_name and secret_region: + try: + aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) + postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] + POSTGRES = json.loads(postgres_secret) + except Exception: + _logger.exception("Failed to load Postgres secret; skipping Postgres writer") + POSTGRES = {"database": ""}
34-36: Optional: avoid f-strings for table identifiers; use psycopg2.sql.Identifier.Prevents accidental SQL injection if table names ever become dynamic inputs.
Example for postgres_edla_write:
-from psycopg2 import sql -# ... -cursor.execute( - f""" - INSERT INTO {table} - ( ... ) - VALUES - ( ... ) - """, - (params...), -) +from psycopg2 import sql +# ... +query = sql.SQL(""" + INSERT INTO {table} ( + event_id, tenant_id, source_app, source_app_version, environment, + timestamp_event, catalog_id, operation, "location", "format", + format_options, additional_info + ) VALUES ( + %s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s + ) +""").format(table=sql.Identifier(table)) +cursor.execute(query, (params...))Apply similarly for runs/jobs/test.
Also applies to: 86-87, 123-124, 159-160
src/event_gate_lambda.py (2)
170-179: Make header extraction case-insensitive; support common “authorization” key.-def extract_token(eventHeaders): - # Initial implementation used bearer header directly - if "bearer" in eventHeaders: - return eventHeaders["bearer"] - - if "Authorization" in eventHeaders and eventHeaders["Authorization"].startswith("Bearer "): - return eventHeaders["Authorization"][len("Bearer ") :] +def extract_token(eventHeaders): + headers = {k.lower(): v for k, v in eventHeaders.items()} + if "bearer" in headers: + return headers["bearer"] + if "authorization" in headers and headers["authorization"].startswith("Bearer "): + return headers["authorization"][len("Bearer ") :] return "" # Will result in 401
34-39: Prefer package-relative imports over mutating sys.path.Make src a package and use from . import writer_*. Helps mypy/importers.
-sys.path.append(os.path.join(os.path.dirname(__file__))) - -import writer_eventbridge -import writer_kafka -import writer_postgres +from . import writer_eventbridge, writer_kafka, writer_postgresAlso add an init.py in src/ and set mypy_path=src if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/CODEOWNERS(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).pylintrc(1 hunks)DEVELOPER.md(1 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)src/__init__.py(1 hunks)src/event_gate_lambda.py(7 hunks)src/requirements.txt(0 hunks)src/writer_eventbridge.py(1 hunks)src/writer_kafka.py(1 hunks)src/writer_postgres.py(7 hunks)tests/conftest.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/requirements.txt
🧰 Additional context used
🧬 Code graph analysis (4)
src/writer_eventbridge.py (2)
src/writer_kafka.py (2)
init(7-30)write(33-51)src/writer_postgres.py (2)
init(13-29)write(189-221)
src/writer_kafka.py (2)
src/writer_eventbridge.py (1)
write(18-42)tests/test_event_gate_lambda.py (2)
produce(15-18)flush(19-20)
src/writer_postgres.py (3)
src/writer_eventbridge.py (1)
write(18-42)src/writer_kafka.py (1)
write(33-51)tests/test_writer_postgres.py (1)
execute(11-12)
src/event_gate_lambda.py (1)
tests/test_event_gate_lambda.py (1)
read(39-44)
🪛 markdownlint-cli2 (0.17.2)
DEVELOPER.md
9-9: Link fragments should be valid
(MD051, link-fragments)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.12.2)
src/writer_eventbridge.py
38-38: Do not catch blind exception: Exception
(BLE001)
39-39: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
39-39: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/writer_kafka.py
41-41: Unused lambda argument: msg
(ARG005)
47-47: Do not catch blind exception: Exception
(BLE001)
48-48: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
48-48: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/writer_postgres.py
8-8: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
217-217: Do not catch blind exception: Exception
(BLE001)
218-218: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
218-218: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/event_gate_lambda.py
128-128: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: Build and Test
src/writer_eventbridge.py
[error] 3-66: Multiple mypy errors detected in this file: missing boto3 stubs; undefined _logger, aws_eventbridge, EVENT_BUS_ARN; import-not-found for writer_kafka and writer_postgres.
src/writer_kafka.py
[error] 3-48: Multiple mypy errors detected in this file: missing type stubs for boto3 and confluent_kafka; undefined symbols _logger and kafka_producer; import-not-found for writer_eventbridge and writer_postgres.
src/writer_postgres.py
[error] 4-218: Multiple mypy errors detected in this file: Skipping analyzing boto3/botocore.exceptions due to missing type stubs; library stubs not installed for psycopg2 (types-psycopg2 recommended); undefined symbols _logger and POSTGRES.
src/event_gate_lambda.py
[error] 23-127: Multiple mypy errors detected in this file: missing stubs for boto3/requests/jsonschema; import-not-found for writer_eventbridge/writer_kafka/writer_postgres; incompatible type for 2nd argument.
🔇 Additional comments (11)
pyproject.toml (2)
2-3: Black config looks good and matches pylint line-length.
7-7: Coverage omit for tests is fine.Excluding tests from coverage computation is standard.
.github/CODEOWNERS (1)
1-1: Confirm owner change is intentional and reflected in branch protection.Removing an owner changes review requirements. Ensure branch rules still enforce the desired approvals.
requirements.txt (1)
1-13: All pinned versions exist on PyPI Verified that black 25.1.0, pytest-cov 6.3.0, pylint 3.3.8, and mypy 1.17.1 are all available; no changes needed..pylintrc (3)
350-351: Line length matches Black’s 120 — good.
456-457: Timeouts list for requests is a nice safeguard.
96-97: Confirm runtime Python is 3.11.py-version=3.11 is set; ensure Lambda containers and local tooling use 3.11 to avoid rule mismatches.
src/__init__.py (1)
1-1: LGTM: package docstring addedNo runtime changes. Good housekeeping.
src/writer_postgres.py (3)
32-81: Good shift to parametrized SQL (inserts).Executes SQL with bound parameters instead of f-strings. Safer and cleaner.
83-154: Runs/jobs inserts look correct and consistently parameterized.No issues spotted; commit/transaction handling is fine.
156-186: No action: timestamp mapping aligns with schema.The
postgres_test_writefunction targets thetimestamp_eventcolumn but sources its value frommessage["timestamp"]—this matchesconf/topic_test.json(which defines and requirestimestamp) and is covered by tests intests/test_writer_postgres.py. The default writer usesmessage["timestamp_event"]for schemas (e.g.,conf/topic_dlchange.json) that expecttimestamp_event. All mappings are consistent and tested.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/writer_eventbridge.py (2)
35-42: Tighten error handling and log failed entries with contextLog only failed entries, use logger.exception, and catch boto-specific exceptions explicitly to satisfy linters and improve diagnostics.
Apply:
- if response["FailedEntryCount"] > 0: - msg = str(response) - _logger.error(msg) - return False, msg - except Exception as e: - err_msg = f"The EventBridge writer failed with unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + if response["FailedEntryCount"] > 0: + failures = [e for e in response.get("Entries", []) if e.get("ErrorCode")] + _logger.error("EventBridge put_events failed: %s", failures) + return False, "EventBridge put_events failed" + except (ClientError, BotoCoreError) as e: + _logger.exception("The EventBridge writer failed with AWS error") + return False, f"AWS error: {e}" + except Exception: + _logger.exception("The EventBridge writer failed with unknown error") + return False, "Unknown error"
19-23: Unify write() return contract across all writers
writer_postgres.write and writer_eventbridge.write return(bool, Optional[str]), but writer_kafka.write only returns a singlebool(writer_kafka.py lines 46, 49, 51). In event_gate_lambda.py (line 144) you’re unpacking two values from writer_kafka.write, which will raise a runtime error. Change writer_kafka.write to return(ok, err: Optional[str])on every code path and update any affected tests or call sites.src/writer_postgres.py (2)
1-11: Resolve mypy and Ruff issues: type globals, remove unused noqa, ignore untyped importsCurrent imports/globals trip mypy (undefined names, untyped libs) and Ruff (unused noqa).
Apply:
-import json -import os - -import boto3 -from botocore.exceptions import ClientError +import json +import os +import logging +from typing import Any, Dict, Optional +import boto3 # type: ignore[import-untyped] +# Remove if unused elsewhere; otherwise catch specific errors. +# from botocore.exceptions import ClientError # type: ignore[import-untyped] + +_logger: logging.Logger = logging.getLogger(__name__) +POSTGRES: Dict[str, Any] = {"database": ""} try: - import psycopg2 # noqa: F401 + import psycopg2 # type: ignore[import-untyped] except ImportError: # pragma: no cover - environment without psycopg2 psycopg2 = NoneThis cures the “name-defined” and “import-untyped” errors and removes the unnecessary noqa.
189-197: Type write() signature and early-return branchesClarify the API and satisfy mypy.
Apply:
-def write(topicName, message): +def write(topicName: str, message: Dict[str, Any]) -> tuple[bool, Optional[str]]:No functional change; helps type checking throughout tests and callers.
🧹 Nitpick comments (18)
src/writer_postgres.py (2)
32-81: Optional: build identifiers safely with psycopg2.sqlTable names are interpolated via f-strings. If they ever become non-constants, use psycopg2.sql to avoid identifier injection.
Example (apply similarly to other writers):
-from typing import Any, Dict, Optional +from typing import Any, Dict, Optional +from psycopg2 import sql # type: ignore[import-untyped] @@ - cursor.execute( - f""" - INSERT INTO {table} + cursor.execute( + sql.SQL(""" + INSERT INTO {} ( event_id, @@ - )""", + )""").format(sql.Identifier(table)), ( message["event_id"], ... ), )
4-5: Remove unused ClientError import (or use it in a targeted except)It’s unused in this file; drop it to keep linters quiet.
-from botocore.exceptions import ClientErrortests/test_writer_postgres.py (5)
5-6: Drop unused import-import pytest -from unittest.mock import patch +import pytest
112-122: Fixture arg not used; simplify signature to avoid ARG001 noisereset_state doesn’t use monkeypatch; removing it quiets linters without behavior change.
-@pytest.fixture -def reset_state(monkeypatch): +@pytest.fixture +def reset_state():
146-151: Silence ARG002 by marking unused kwargs- def connect(self, **kwargs): + def connect(self, **_kwargs): return DummyConnection(self.store)
152-176: Prefix unused test fixture params with underscore to appease linters (optional)The tests rely on reset_state for isolation but don’t reference it. Renaming avoids ARG001 noise.
Example changes:
-def test_write_skips_when_no_database(reset_state): +def test_write_skips_when_no_database(_reset_state): @@ -def test_write_skips_when_psycopg2_missing(reset_state, monkeypatch): +def test_write_skips_when_psycopg2_missing(_reset_state, monkeypatch): @@ -def test_write_unknown_topic_returns_false(reset_state, monkeypatch): +def test_write_unknown_topic_returns_false(_reset_state, monkeypatch): @@ -def test_write_success_known_topic(reset_state, monkeypatch): +def test_write_success_known_topic(_reset_state, monkeypatch): @@ -def test_write_exception_returns_false(reset_state, monkeypatch): +def test_write_exception_returns_false(_reset_state, monkeypatch): @@ -def test_init_with_secret(monkeypatch, reset_state): +def test_init_with_secret(monkeypatch, _reset_state): @@ -def test_write_dlchange_success(reset_state, monkeypatch): +def test_write_dlchange_success(_reset_state, monkeypatch): @@ -def test_write_runs_success(reset_state, monkeypatch): +def test_write_runs_success(_reset_state, monkeypatch):Alternatively, ignore ARG00x in tests via linter config.
Also applies to: 178-186, 187-198, 199-219
189-196: Optional: silence S105 false positive on test-only env varsThese aren’t secrets; add a per-line ignore if S105 is enabled.
- os.environ["POSTGRES_SECRET_NAME"] = "mysecret" - os.environ["POSTGRES_SECRET_REGION"] = "eu-west-1" + os.environ["POSTGRES_SECRET_NAME"] = "mysecret" # noqa: S105 + os.environ["POSTGRES_SECRET_REGION"] = "eu-west-1" # noqa: S105tests/test_event_gate_lambda.py (11)
24-27: Avoid global sys.modules pollution; move dummy module injection into the fixture and restore it on teardown.Top-level injection of optional deps (and the similar confluent_kafka block above) leaks across the whole test session. Scope it to the fixture and restore afterward.
Apply these diffs:
-# Inject dummy psycopg2 (optional dependency) -if 'psycopg2' not in sys.modules: - sys.modules['psycopg2'] = types.ModuleType('psycopg2')@pytest.fixture(scope="module") def event_gate_module(): - started_patches = [] + started_patches = [] + orig_sysmods = {} + + def inject_dummy_module(name, module): + orig_sysmods[name] = sys.modules.get(name) + sys.modules[name] = moduleThen, just before importing
src.event_gate_lambda, inject the dummies (and remove the top-level confluent_kafka block by moving it here):- # Allow kafka producer patching (already stubbed) but still patch to inspect if needed - start_patch('confluent_kafka.Producer') + # Inject optional deps if missing (scoped to this fixture) + if 'confluent_kafka' not in sys.modules: + dummy_ck = types.ModuleType('confluent_kafka') + class DummyProducer: + def __init__(self, *a, **kw): pass + def produce(self, *a, **kw): + cb = kw.get('callback') + if cb: cb(None, None) + def flush(self): return None + dummy_ck.Producer = DummyProducer + inject_dummy_module('confluent_kafka', dummy_ck) + if 'psycopg2' not in sys.modules: + inject_dummy_module('psycopg2', types.ModuleType('psycopg2')) + + # Allow kafka producer patching (already stubbed) but still patch to inspect if needed + start_patch('confluent_kafka.Producer')And restore after
yield:- for p in started_patches: - p.stop() - patch.stopall() + for p in started_patches: + p.stop() + for name, orig in orig_sysmods.items(): + if orig is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = orig
37-41: Harden the mock response.Set a status code to avoid latent failures if the code checks it.
mock_requests_get = start_patch('requests.get') - mock_requests_get.return_value.json.return_value = {"key": base64.b64encode(b'dummy_der').decode('utf-8')} + mock_requests_get.return_value.status_code = 200 + mock_requests_get.return_value.json.return_value = { + "key": base64.b64encode(b'dummy_der').decode('utf-8') + }
65-69: Consider removing unused patch or assert behavior that needs it.You patch
boto3.clientto an Events client but never assert on it in these tests. Either:
- remove this patch to reduce noise, or
- assert
put_eventswas called in at least one path.
71-72: Redundant patch unless asserted.If you don’t observe calls or behavior from
Producer, this patch can be dropped (Kafka writes are already mocked at the writer layer).
75-79: Don’t call patch.stopall(); rely on explicit stops.
patch.stopall()is redundant here and risks stopping patchers outside this fixture.- for p in started_patches: - p.stop() - patch.stopall() + for p in started_patches: + p.stop()
81-92: Default content type in the event factory.Some handlers rely on
Content-Type: application/json. Set a default while allowing overrides.@pytest.fixture def make_event(): def _make(resource, method='GET', body=None, topic=None, headers=None): + base_headers = {'Content-Type': 'application/json'} + if headers: + base_headers.update(headers) return { 'resource': resource, 'httpMethod': method, - 'headers': headers or {}, + 'headers': base_headers, 'pathParameters': {'topic_name': topic} if topic else {}, 'body': json.dumps(body) if isinstance(body, dict) else body } return _make
147-158: Parametrize writer outcomes to reduce duplication and improve coverage.These three tests differ only by writer outcomes/expectations. Parametrize to keep intent clear and add more combinations cheaply.
Example:
import pytest @pytest.mark.parametrize( "kafka_ok, eb_ok, pg_ok, expected_status, expected_error_types", [ (True, True, True, 202, []), (False, True, True, 500, ['kafka']), (False, False, True, 500, ['eventbridge', 'kafka']), # add (True, False, False) etc. if desired ], ) def test_post_writer_matrix(event_gate_module, make_event, valid_payload, kafka_ok, eb_ok, pg_ok, expected_status, expected_error_types): with patch.object(event_gate_module.jwt, 'decode', return_value={'sub': 'TestUser'}, create=True), \ patch('src.event_gate_lambda.writer_kafka.write', return_value=(kafka_ok, None if kafka_ok else 'k')), \ patch('src.event_gate_lambda.writer_eventbridge.write', return_value=(eb_ok, None if eb_ok else 'e')), \ patch('src.event_gate_lambda.writer_postgres.write', return_value=(pg_ok, None if pg_ok else 'p')): event = make_event('/topics/{topic_name}', method='POST', topic='public.cps.za.test', body=valid_payload, headers={'Authorization':'Bearer token'}) resp = event_gate_module.lambda_handler(event, None) assert resp['statusCode'] == expected_status body = json.loads(resp['body']) if expected_status == 202: assert body['success'] else: assert sorted(e['type'] for e in body['errors']) == sorted(expected_error_types)Also applies to: 159-171, 172-182
183-191: Also test standard Authorization header with lowercase prefix.You cover a lowercase header name; add a case for lowercase "bearer" scheme in the Authorization header.
def test_token_extraction_lowercase_bearer_scheme(event_gate_module, make_event, valid_payload): with patch.object(event_gate_module.jwt, 'decode', return_value={'sub': 'TestUser'}, create=True), \ patch('src.event_gate_lambda.writer_kafka.write', return_value=(True, None)), \ patch('src.event_gate_lambda.writer_eventbridge.write', return_value=(True, None)), \ patch('src.event_gate_lambda.writer_postgres.write', return_value=(True, None)): event = make_event( '/topics/{topic_name}', method='POST', topic='public.cps.za.test', body=valid_payload, headers={'Authorization': 'bearer token'} ) resp = event_gate_module.lambda_handler(event, None) assert resp['statusCode'] == 202
199-210: Token endpoint: assert redirect target shape.Optionally assert
Locationpoints at the configured auth endpoint (prefix/host), to lock contract.
99-105: Ruff S101 in tests: configure an ignore instead of changing asserts.Pytest encourages bare
assert. If Ruff is enabled, ignore S101 for tests.Add to pyproject.toml:
[tool.ruff] # ... [tool.ruff.lint.per-file-ignores] "tests/**.py" = ["S101"]
211-218: Optional: cover the /terminate route safely.Patch
sys.exitto a no-op or to raise a sentinel and assert it was invoked.Example:
def test_terminate_route(event_gate_module, make_event): with patch('src.event_gate_lambda.sys.exit', return_value=None) as exit_mock: resp = event_gate_module.lambda_handler(make_event('/terminate'), None) exit_mock.assert_called_once()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/writer_eventbridge.py(3 hunks)src/writer_postgres.py(8 hunks)tests/test_conf_validation.py(2 hunks)tests/test_event_gate_lambda.py(3 hunks)tests/test_writer_postgres.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_writer_postgres.py (1)
src/writer_postgres.py (5)
init(13-29)postgres_edla_write(32-80)postgres_run_write(83-153)postgres_test_write(156-186)write(189-223)
tests/test_event_gate_lambda.py (1)
src/event_gate_lambda.py (1)
lambda_handler(181-203)
src/writer_postgres.py (1)
tests/test_writer_postgres.py (7)
client(193-194)cursor(137-138)execute(17-18)execute(126-127)connect(149-150)connect(180-181)commit(139-140)
🪛 Ruff (0.12.2)
tests/test_conf_validation.py
26-26: Use of assert detected
(S101)
33-33: Use of assert detected
(S101)
38-38: Use of assert detected
(S101)
40-40: Use of assert detected
(S101)
41-41: Use of assert detected
(S101)
42-42: Use of assert detected
(S101)
46-46: Use of assert detected
(S101)
48-48: Use of assert detected
(S101)
50-50: Use of assert detected
(S101)
52-52: Use of assert detected
(S101)
54-54: Use of assert detected
(S101)
56-56: Use of assert detected
(S101)
57-57: Use of assert detected
(S101)
tests/test_writer_postgres.py
39-39: Use of assert detected
(S101)
41-41: Use of assert detected
(S101)
42-42: Use of assert detected
(S101)
43-43: Use of assert detected
(S101)
44-44: Use of assert detected
(S101)
45-45: Use of assert detected
(S101)
46-46: Use of assert detected
(S101)
63-63: Use of assert detected
(S101)
64-64: Use of assert detected
(S101)
65-65: Use of assert detected
(S101)
66-66: Use of assert detected
(S101)
85-85: Use of assert detected
(S101)
87-87: Use of assert detected
(S101)
88-88: Use of assert detected
(S101)
90-90: Use of assert detected
(S101)
91-91: Use of assert detected
(S101)
104-104: Use of assert detected
(S101)
106-106: Use of assert detected
(S101)
107-107: Use of assert detected
(S101)
108-108: Use of assert detected
(S101)
113-113: Unused function argument: monkeypatch
(ARG001)
149-149: Unused method argument: kwargs
(ARG002)
152-152: Unused function argument: reset_state
(ARG001)
155-155: Use of assert detected
(S101)
157-157: Unused function argument: reset_state
(ARG001)
161-161: Use of assert detected
(S101)
163-163: Unused function argument: reset_state
(ARG001)
168-168: Use of assert detected
(S101)
170-170: Unused function argument: reset_state
(ARG001)
176-176: Use of assert detected
(S101)
178-178: Unused function argument: reset_state
(ARG001)
180-180: Unused method argument: kwargs
(ARG002)
185-185: Use of assert detected
(S101)
187-187: Unused function argument: reset_state
(ARG001)
189-189: Possible hardcoded password assigned to: "POSTGRES_SECRET_NAME"
(S105)
190-190: Possible hardcoded password assigned to: "POSTGRES_SECRET_REGION"
(S105)
191-191: Unused lambda argument: SecretId
(ARG005)
193-193: Unused method argument: service_name
(ARG002)
193-193: Unused method argument: region_name
(ARG002)
197-197: Use of assert detected
(S101)
199-199: Unused function argument: reset_state
(ARG001)
208-208: Use of assert detected
(S101)
210-210: Unused function argument: reset_state
(ARG001)
219-219: Use of assert detected
(S101)
tests/test_event_gate_lambda.py
102-102: Use of assert detected
(S101)
104-104: Use of assert detected
(S101)
109-109: Use of assert detected
(S101)
111-111: Use of assert detected
(S101)
116-116: Use of assert detected
(S101)
123-123: Use of assert detected
(S101)
125-125: Use of assert detected
(S101)
126-126: Use of assert detected
(S101)
132-132: Use of assert detected
(S101)
134-134: Use of assert detected
(S101)
141-141: Use of assert detected
(S101)
143-143: Use of assert detected
(S101)
154-154: Use of assert detected
(S101)
156-156: Use of assert detected
(S101)
157-157: Use of assert detected
(S101)
166-166: Use of assert detected
(S101)
168-168: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
170-170: Use of assert detected
(S101)
179-179: Use of assert detected
(S101)
181-181: Use of assert detected
(S101)
190-190: Use of assert detected
(S101)
195-195: Use of assert detected
(S101)
197-197: Use of assert detected
(S101)
202-202: Use of assert detected
(S101)
203-203: Use of assert detected
(S101)
208-208: Use of assert detected
(S101)
209-209: Use of assert detected
(S101)
215-215: Use of assert detected
(S101)
217-217: Use of assert detected
(S101)
src/writer_eventbridge.py
40-40: Use explicit conversion flag
Replace with conversion flag
(RUF010)
41-41: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer_postgres.py
8-8: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
218-218: Do not catch blind exception: Exception
(BLE001)
219-219: Use explicit conversion flag
Replace with conversion flag
(RUF010)
220-220: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 GitHub Actions: Build and Test
src/writer_eventbridge.py
[error] 3-3: Mypy: Skipping analyzing 'boto3': module is installed, but missing library stubs or py.typed marker [import-untyped]
[error] 11-11: Mypy: Name '_logger' is not defined [name-defined]
[error] 13-13: Mypy: Name 'aws_eventbridge' is not defined [name-defined]
[error] 14-14: Mypy: Name 'EVENT_BUS_ARN' is not defined [name-defined]
[error] 15-15: Mypy: Name '_logger' is not defined [name-defined]
[error] 19-19: Mypy: Name 'EVENT_BUS_ARN' is not defined [name-defined]
[error] 20-20: Mypy: Name '_logger' is not defined [name-defined]
[error] 24-24: Mypy: Name '_logger' is not defined [name-defined]
[error] 25-25: Mypy: Name 'aws_eventbridge' is not defined [name-defined]
[error] 31-31: Mypy: Name 'EVENT_BUS_ARN' is not defined [name-defined]
[error] 37-37: Mypy: Name '_logger' is not defined [name-defined]
[error] 41-41: Mypy: Name '_logger' is not defined [name-defined]
[error] 41-41: Mypy: Name 'aws_eventbridge' is not defined [name-defined]
[error] 41-41: Mypy: Name 'aws_eventbridge' is not defined [name-defined]
src/writer_postgres.py
[error] 4-4: Mypy: Skipping analyzing 'boto3': module is installed, but missing library stubs or py.typed marker [import-untyped]
[error] 5-5: Mypy: Skipping analyzing 'botocore.exceptions': module is installed, but missing library stubs or py.typed marker [import-untyped]
[error] 8-8: Mypy: Library stubs not installed for 'psycopg2' [import-untyped]
[error] 17-17: Mypy: Name '_logger' is not defined [name-defined]
[error] 25-25: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 27-27: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 29-29: Mypy: Name '_logger' is not defined [name-defined]
[error] 33-33: Mypy: Name '_logger' is not defined [name-defined]
[error] 84-84: Mypy: Name '_logger' is not defined [name-defined]
[error] 157-157: Mypy: Name '_logger' is not defined [name-defined]
[error] 191-191: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 192-192: Mypy: Name '_logger' is not defined [name-defined]
[error] 195-195: Mypy: Name '_logger' is not defined [name-defined]
[error] 199-199: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 200-200: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 201-201: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 202-202: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 203-203: Mypy: Name 'POSTGRES' is not defined [name-defined]
[error] 214-214: Mypy: Name '_logger' is not defined [name-defined]
[error] 220-220: Mypy: Name '_logger' is not defined [name-defined]
🔇 Additional comments (1)
tests/test_event_gate_lambda.py (1)
28-36: Nice fixture and patch helper.The module-scoped fixture +
start_patchhelper is clean and keeps setup/teardown centralized.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/writer_eventbridge.py (1)
33-60: Type write(); improve failure logging; catch boto errors explicitlyReturn type aligns with other writers. Log only failed entries and use exception() for stack traces.
-def write(topicName, message): +def write(topicName: str, message: Dict[str, Any]) -> tuple[bool, str | None]: @@ - if response["FailedEntryCount"] > 0: - msg = str(response) - _logger.error(msg) - return False, msg - except Exception as e: - err_msg = f"The EventBridge writer failed with unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + if response["FailedEntryCount"] > 0: + failures = [e for e in response.get("Entries", []) if "ErrorCode" in e] + _logger.error("EventBridge put_events failed: %s", failures) + return False, str(failures) + except (BotoCoreError, ClientError): + _logger.exception("The EventBridge writer failed with AWS error") + return False, "The EventBridge writer failed with AWS error" + except Exception: + _logger.exception("The EventBridge writer failed with unknown error") + return False, "The EventBridge writer failed with unknown error"tests/test_event_gate_lambda.py (1)
24-38: Avoid shadowing real installations of confluent_kafka.Checking only sys.modules can overwrite a real installed package. Prefer importlib.util.find_spec and use sys.modules.setdefault to avoid clobbering.
Apply this diff, and add the import shown below:
-if 'confluent_kafka' not in sys.modules: +if importlib.util.find_spec('confluent_kafka') is None: dummy_ck = types.ModuleType('confluent_kafka') class DummyProducer: # minimal interface def __init__(self, *a, **kw): pass def produce(self, *a, **kw): cb = kw.get('callback') if cb: cb(None, None) def flush(self): return None dummy_ck.Producer = DummyProducer - sys.modules['confluent_kafka'] = dummy_ck + sys.modules.setdefault('confluent_kafka', dummy_ck)Outside this hunk, add:
+import importlib.util
♻️ Duplicate comments (9)
src/writer_kafka.py (2)
28-42: Prevent KeyError: validate all SASL/SSL config keys before updateYou access multiple CONFIG keys but guard only two; this will explode at runtime if any are missing.
- if "kafka_sasl_kerberos_principal" in CONFIG and "kafka_ssl_key_path" in CONFIG: - producer_config.update( + if "kafka_sasl_kerberos_principal" in CONFIG and "kafka_ssl_key_path" in CONFIG: + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in CONFIG] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": CONFIG["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": CONFIG["kafka_sasl_kerberos_principal"], "ssl.ca.location": CONFIG["kafka_ssl_ca_path"], "ssl.certificate.location": CONFIG["kafka_ssl_cert_path"], "ssl.key.location": CONFIG["kafka_ssl_key_path"], "ssl.key.password": CONFIG["kafka_ssl_key_password"], } )
51-64: Fix Ruff hits and improve diagnostics in delivery callback pathAddress ARG005/BLE001/TRY400/RUF010 and make errors actionable.
- error = [] + errors: list[object] = [] @@ - callback=lambda err, msg: error.append(err) if err is not None else None, + callback=lambda err, _msg: errors.append(err) if err is not None else None, ) - kafka_producer.flush() - if error: - _logger.error(str(error)) - return False - except Exception as e: - _logger.error(f"The Kafka writer failed with unknown error: {str(e)}") - return False + kafka_producer.flush(timeout=10.0) + if errors: + _logger.error("Kafka delivery errors: %s", errors) + return False, ", ".join(map(str, errors)) + except Exception: + _logger.exception("The Kafka writer failed with unknown error") + return False, "The Kafka writer failed with unknown error"src/writer_postgres.py (2)
203-239: Type write(); log exceptions with stack traces; keep return tupleAligns with QA goals and existing tests while improving diagnostics.
-def write(topicName, message): +def write(topicName: str, message: Dict[str, Any]) -> tuple[bool, str | None]: @@ - except Exception as e: # pragma: no cover - defensive (still tested though) - err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + except Exception as e: # pragma: no cover - defensive (still tested though) + _logger.exception("The Postgres writer failed with unknown error") + return False, f"The Postgres writer failed with unknown error: {e}"
19-26: Silence mypy/ruff: add typing, remove unused noqa, and ignore untyped importsCurrent pipeline flags untyped imports and undefined globals.
+import logging +from typing import Any, Dict, Optional -import boto3 -from botocore.exceptions import ClientError +import boto3 # type: ignore[import-untyped] +from botocore.exceptions import ClientError # type: ignore[import-untyped] @@ -try: - import psycopg2 # noqa: F401 +try: + import psycopg2 # type: ignore[import-untyped] except ImportError: # pragma: no cover - environment without psycopg2 - psycopg2 = None + psycopg2 = None # type: Optional[Any] + +_logger: logging.Logger = logging.getLogger(__name__) +POSTGRES: Dict[str, Any] = {"database": ""}src/writer_eventbridge.py (2)
21-31: Type public API: init() signatureAnnotate parameters and return type.
-def init(logger, CONFIG): +def init(logger: logging.Logger, CONFIG: Dict[str, str]) -> None:
16-20: Declare typed globals and ignore untyped imports to fix mypy errorsPrevents “name not defined” and import-untyped failures.
+import logging +from typing import Any, Dict -import boto3 +import boto3 # type: ignore[import-untyped] +from botocore.exceptions import BotoCoreError, ClientError # type: ignore[import-untyped] + +_logger: logging.Logger = logging.getLogger(__name__) +EVENT_BUS_ARN: str = "" +aws_eventbridge: Any | None = Nonetests/test_conf_validation.py (1)
59-61: Parametrization hides “no topic files” failure — add a dedicated existence test.When the glob is empty, pytest collects 0 tests and the assert never runs. Add a separate test to enforce presence of topic_*.json.
+def test_topic_json_files_exist(): + files = glob(os.path.join(CONF_DIR, "topic_*.json")) + assert files, "No topic_*.json files found"src/event_gate_lambda.py (2)
40-41: Remove insecure TLS settings; add timeouts.Do not disable TLS verification or warnings. Use default trust store; support custom CAs via REQUESTS_CA_BUNDLE/AWS_CA_BUNDLE envs. Add a reasonable timeout to HTTP calls.
-import urllib3 -urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) +# Use default TLS verification; allow REQUESTS_CA_BUNDLE / AWS_CA_BUNDLE if needed. -aws_s3 = boto3.Session().resource("s3", verify=False) +aws_s3 = boto3.Session().resource("s3") -token_public_key_encoded = requests.get(CONFIG["token_public_key_url"], verify=False).json()["key"] +token_public_key_encoded = requests.get(CONFIG["token_public_key_url"], timeout=10).json()["key"]Also applies to: 65-66, 78-81
124-131: Catch PyJWT-specific errors instead of blind except.Return 401 only for token issues; let unexpected errors bubble.
- try: - token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) - except Exception: - return _error_response(401, "auth", "Invalid or missing token") + try: + token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) + except jwt.exceptions.InvalidTokenError: + return _error_response(401, "auth", "Invalid or missing token")
🧹 Nitpick comments (18)
src/writer_kafka.py (1)
44-45: Optional: set client.id and acks for observability/reliabilityConsider adding producer_config["client.id"] and ["acks"]="all" based on your reliability needs.
tests/test_conf_validation.py (1)
35-43: Ruff S101 (assert) in tests — configure ignore instead of rewriting.Asserts are idiomatic in pytest. Suppress S101 (and similar test-only rules) in tests via pyproject.
+[tool.ruff] +target-version = "py311" + +[tool.ruff.lint.per-file-ignores] +"tests/**" = ["S101"]Also applies to: 44-49, 50-57, 59-73
README.md (2)
127-132: Specify code-fence language for the shell snippet.Adds syntax highlighting; fixes MD040.
-``` +```bash python -m venv .venv source .venv/bin/activate pip install -r requirements.txt pytest -q--- `165-172`: **Add blank lines around the table.** Satisfies MD058 and improves readability. ```diff -## Troubleshooting -| Symptom | Possible Cause | Action | +## Troubleshooting + +| Symptom | Possible Cause | Action | |---------|----------------|--------| ... -| Lambda keeps old config | Warm container | Call `/terminate` (internal) to force cold start | +| Lambda keeps old config | Warm container | Call `/terminate` (internal) to force cold start | +tests/test_writer_postgres.py (3)
161-166: Silence ARG002 by naming unused kwargs as _kwargs.Keeps tests clean without disabling the rule globally.
-class DummyPsycopg: +class DummyPsycopg: def __init__(self, store): self.store = store - def connect(self, **kwargs): + def connect(self, **_kwargs): return DummyConnection(self.store)
127-137: Ruff rules in tests (ARG001/ARG002/S101/S105) — prefer per-file ignores.Fixtures used by name trigger ARG001; asserts are fine in pytest; S105 here is a false positive. Recommend test-only ignores.
+[tool.ruff.lint.per-file-ignores] +"tests/**" = ["S101", "ARG001", "ARG002", "ARG005", "S105"]Also applies to: 167-235
185-192: Optionally assert transaction commit.You already track commit_called; asserting it strengthens the “success” path.
ok, err = writer_postgres.write("public.cps.za.test", message) -assert ok and err is None and len(store) == 1 +assert ok and err is None and len(store) == 1 +# Optionally, expose the DummyConnection instance to assert: +# assert any(getattr(c, "commit_called", False) for c in connections_captured)src/event_gate_lambda.py (6)
98-100: Return proper content type for /api.Expose OpenAPI YAML with a text/yaml content-type.
-def get_api(): - return {"statusCode": 200, "body": API} +def get_api(): + return {"statusCode": 200, "headers": {"Content-Type": "text/yaml"}, "body": API}
68-76: S3 loader: add minimal error handling and log context.Defensive read to avoid opaque 500s during cold start if object missing.
-ACCESS = json.loads(aws_s3.Bucket(bucket_name).Object(bucket_object).get()["Body"].read().decode("utf-8")) +obj = aws_s3.Bucket(bucket_name).Object(bucket_object) +ACCESS = json.loads(obj.get()["Body"].read().decode("utf-8"))Optionally wrap in try/except and log bucket/key before re-raising.
107-114: Stable /topics ordering.Sort keys to keep responses deterministic.
- "body": json.dumps([topicName for topicName in TOPICS]), + "body": json.dumps(sorted(TOPICS.keys())),
30-39: Mypy: avoid sys.path hacking; configure mypy path or package src.Short-term: set mypy_path=src. Long-term: make src a package and use relative imports.
-# Resolve project root (parent directory of this file's directory) -_PROJECT_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) -_CONF_DIR = os.path.join(_PROJECT_ROOT, "conf") - -sys.path.append(os.path.join(os.path.dirname(__file__))) +# Resolve project root (parent directory of this file's directory) +_PROJECT_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) +_CONF_DIR = os.path.join(_PROJECT_ROOT, "conf")pyproject.toml (mypy section):
[tool.mypy] python_version = "3.11" mypy_path = "src" ignore_missing_imports = false [tool.mypy-requests.*] ignore_missing_imports = true [tool.mypy-jsonschema.*] ignore_missing_imports = true [tool.mypy-boto3.*] ignore_missing_imports = true
124-131: Mypy: annotate token/public key to satisfy typing.Prevents “incompatible argument type” at decode.
+from typing import Any, Dict +TOKEN_PUBLIC_KEY: Any ... - try: - token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) + try: + token: Dict[str, Any] = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"])
181-197: Return 400 for invalid JSON payloads.Differentiate bad JSON from internal errors.
- if event["httpMethod"] == "POST": - return post_topic_message( - event["pathParameters"]["topic_name"].lower(), - json.loads(event["body"]), - extract_token(event["headers"]), - ) + if event["httpMethod"] == "POST": + try: + body = json.loads(event["body"]) + except Exception: + return _error_response(400, "request", "Invalid JSON body") + return post_topic_message( + event["pathParameters"]["topic_name"].lower(), + body, + extract_token(event["headers"]), + )tests/test_event_gate_lambda.py (5)
52-57: Harden requests mock for future-proofing.If the production code starts calling raise_for_status or inspecting status_code, current mock will break. Set them explicitly.
mock_requests_get = start_patch('requests.get') -mock_requests_get.return_value.json.return_value = {"key": base64.b64encode(b'dummy_der').decode('utf-8')} +mock_resp = mock_requests_get.return_value +mock_resp.status_code = 200 +mock_resp.raise_for_status = MagicMock() +mock_resp.json.return_value = {"key": base64.b64encode(b'dummy_der').decode('utf-8')}
76-87: Kafka patching is redundant (optional).You already stub the module; additionally patching Producer is fine but unnecessary unless you need call assertions. If not asserted anywhere, consider dropping for simplicity.
198-206: Add coverage for 'authorization' header with lowercase bearer.You test header key 'bearer'. Also cover 'authorization' with lowercase scheme to guard case-insensitive parsing.
Add alongside this test:
def test_token_extraction_authorization_lowercase_scheme(event_gate_module, make_event, valid_payload): with patch.object(event_gate_module.jwt, 'decode', return_value={'sub': 'TestUser'}, create=True), \ patch('src.event_gate_lambda.writer_kafka.write', return_value=(True, None)), \ patch('src.event_gate_lambda.writer_eventbridge.write', return_value=(True, None)), \ patch('src.event_gate_lambda.writer_postgres.write', return_value=(True, None)): event = make_event('/topics/{topic_name}', method='POST', topic='public.cps.za.test', body=valid_payload, headers={'authorization':'bearer token'}) resp = event_gate_module.lambda_handler(event, None) assert resp['statusCode'] == 202
226-233: Consider testing the '/terminate' path explicitly.Ensure sys.exit is invoked without killing the test process.
Add:
def test_terminate_route(event_gate_module, make_event): with patch('src.event_gate_lambda.sys.exit', side_effect=SystemExit) as exit_mock: event = make_event('/terminate') with pytest.raises(SystemExit): event_gate_module.lambda_handler(event, None) exit_mock.assert_called_once_with("TERMINATING")
117-224: Ruff S101 assertions in tests (only if Ruff is enabled).If Ruff runs in CI, ignore S101 in tests to keep idiomatic pytest asserts.
Example pyproject.toml:
[tool.ruff] extend-exclude = ["build", "dist"] [tool.ruff.lint.per-file-ignores] "tests/**" = ["S101"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)src/__init__.py(1 hunks)src/event_gate_lambda.py(7 hunks)src/writer_eventbridge.py(3 hunks)src/writer_kafka.py(1 hunks)src/writer_postgres.py(8 hunks)tests/conftest.py(1 hunks)tests/test_conf_validation.py(2 hunks)tests/test_event_gate_lambda.py(3 hunks)tests/test_writer_postgres.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/init.py
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/test_writer_postgres.py (1)
src/writer_postgres.py (5)
init(28-44)postgres_edla_write(47-95)postgres_run_write(98-168)postgres_test_write(171-201)write(204-238)
src/writer_postgres.py (1)
tests/test_writer_postgres.py (7)
client(208-209)cursor(152-153)execute(32-33)execute(141-142)connect(164-165)connect(195-196)commit(154-155)
src/writer_eventbridge.py (1)
src/writer_postgres.py (1)
init(28-44)
src/event_gate_lambda.py (1)
tests/test_event_gate_lambda.py (1)
read(60-65)
src/writer_kafka.py (3)
src/writer_eventbridge.py (2)
init(21-30)write(33-59)src/writer_postgres.py (2)
init(28-44)write(204-238)tests/test_event_gate_lambda.py (2)
produce(30-33)flush(34-35)
tests/test_event_gate_lambda.py (1)
src/event_gate_lambda.py (1)
lambda_handler(181-203)
🪛 Ruff (0.12.2)
tests/test_conf_validation.py
41-41: Use of assert detected
(S101)
48-48: Use of assert detected
(S101)
53-53: Use of assert detected
(S101)
55-55: Use of assert detected
(S101)
56-56: Use of assert detected
(S101)
57-57: Use of assert detected
(S101)
61-61: Use of assert detected
(S101)
63-63: Use of assert detected
(S101)
65-65: Use of assert detected
(S101)
67-67: Use of assert detected
(S101)
69-69: Use of assert detected
(S101)
71-71: Use of assert detected
(S101)
72-72: Use of assert detected
(S101)
tests/test_writer_postgres.py
54-54: Use of assert detected
(S101)
56-56: Use of assert detected
(S101)
57-57: Use of assert detected
(S101)
58-58: Use of assert detected
(S101)
59-59: Use of assert detected
(S101)
60-60: Use of assert detected
(S101)
61-61: Use of assert detected
(S101)
78-78: Use of assert detected
(S101)
79-79: Use of assert detected
(S101)
80-80: Use of assert detected
(S101)
81-81: Use of assert detected
(S101)
100-100: Use of assert detected
(S101)
102-102: Use of assert detected
(S101)
103-103: Use of assert detected
(S101)
105-105: Use of assert detected
(S101)
106-106: Use of assert detected
(S101)
119-119: Use of assert detected
(S101)
121-121: Use of assert detected
(S101)
122-122: Use of assert detected
(S101)
123-123: Use of assert detected
(S101)
128-128: Unused function argument: monkeypatch
(ARG001)
164-164: Unused method argument: kwargs
(ARG002)
167-167: Unused function argument: reset_state
(ARG001)
170-170: Use of assert detected
(S101)
172-172: Unused function argument: reset_state
(ARG001)
176-176: Use of assert detected
(S101)
178-178: Unused function argument: reset_state
(ARG001)
183-183: Use of assert detected
(S101)
185-185: Unused function argument: reset_state
(ARG001)
191-191: Use of assert detected
(S101)
193-193: Unused function argument: reset_state
(ARG001)
195-195: Unused method argument: kwargs
(ARG002)
200-200: Use of assert detected
(S101)
202-202: Unused function argument: reset_state
(ARG001)
204-204: Possible hardcoded password assigned to: "POSTGRES_SECRET_NAME"
(S105)
205-205: Possible hardcoded password assigned to: "POSTGRES_SECRET_REGION"
(S105)
206-206: Unused lambda argument: SecretId
(ARG005)
208-208: Unused method argument: service_name
(ARG002)
208-208: Unused method argument: region_name
(ARG002)
212-212: Use of assert detected
(S101)
214-214: Unused function argument: reset_state
(ARG001)
223-223: Use of assert detected
(S101)
225-225: Unused function argument: reset_state
(ARG001)
234-234: Use of assert detected
(S101)
src/writer_postgres.py
23-23: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
233-233: Do not catch blind exception: Exception
(BLE001)
234-234: Use explicit conversion flag
Replace with conversion flag
(RUF010)
235-235: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer_eventbridge.py
55-55: Use explicit conversion flag
Replace with conversion flag
(RUF010)
56-56: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/event_gate_lambda.py
128-128: Do not catch blind exception: Exception
(BLE001)
src/writer_kafka.py
56-56: Unused lambda argument: msg
(ARG005)
62-62: Do not catch blind exception: Exception
(BLE001)
63-63: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
63-63: Use explicit conversion flag
Replace with conversion flag
(RUF010)
tests/test_event_gate_lambda.py
117-117: Use of assert detected
(S101)
119-119: Use of assert detected
(S101)
124-124: Use of assert detected
(S101)
126-126: Use of assert detected
(S101)
131-131: Use of assert detected
(S101)
138-138: Use of assert detected
(S101)
140-140: Use of assert detected
(S101)
141-141: Use of assert detected
(S101)
147-147: Use of assert detected
(S101)
149-149: Use of assert detected
(S101)
156-156: Use of assert detected
(S101)
158-158: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
171-171: Use of assert detected
(S101)
172-172: Use of assert detected
(S101)
181-181: Use of assert detected
(S101)
183-183: Use of assert detected
(S101)
184-184: Use of assert detected
(S101)
185-185: Use of assert detected
(S101)
194-194: Use of assert detected
(S101)
196-196: Use of assert detected
(S101)
205-205: Use of assert detected
(S101)
210-210: Use of assert detected
(S101)
212-212: Use of assert detected
(S101)
217-217: Use of assert detected
(S101)
218-218: Use of assert detected
(S101)
223-223: Use of assert detected
(S101)
224-224: Use of assert detected
(S101)
230-230: Use of assert detected
(S101)
232-232: Use of assert detected
(S101)
🪛 markdownlint-cli2 (0.17.2)
README.md
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
165-165: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 GitHub Actions: Build and Test
src/writer_postgres.py
[error] 1-1: Mypy: multiple type errors detected in src/writer_postgres.py (missing type stubs for boto3 and psycopg2; undefined _logger and POSTGRES).
src/writer_eventbridge.py
[error] 1-1: Mypy: multiple type errors detected in src/writer_eventbridge.py (import-untyped; undefined _logger / aws_eventbridge / EVENT_BUS_ARN).
src/event_gate_lambda.py
[error] 1-1: Mypy: multiple type errors detected in src/event_gate_lambda.py (import-untyped; missing stubs for requests/jsonschema; missing writer modules; incompatible argument type at line 127).
src/writer_kafka.py
[error] 1-1: Mypy: multiple type errors detected in src/writer_kafka.py (import-untyped modules; undefined _logger / kafka_producer).
🔇 Additional comments (8)
src/writer_eventbridge.py (1)
33-37: Resolve tuple consistency across writersAll three writers—Kafka, EventBridge, and Postgres—return
(success: bool, error: Optional[str]), and every call site unpacks two values. No changes needed.README.md (1)
159-163: Typo matches actual script name Thescripts/prepare.deplyoment.shreference in README.md aligns with the file on disk; no changes needed unless you decide to rename it.tests/test_event_gate_lambda.py (6)
43-51: Fixture patch helper looks good.Nice pre-import patching and centralized cleanup via started_patches; solid foundation for deterministic imports.
96-107: Event factory is clean.Good normalization of body and headers; pathParameters handling matches handler expectations.
114-132: GET topics and schema tests are solid.Happy path and 404 covered succinctly.
135-159: Auth and validation branches covered well.401, 403, and 400 assertions match the handler contract.
162-197: POST writers: success and aggregation logic validated.202 on full success, 500 with typed errors on partial/multi failures—great coverage.
207-225: Route and API/token endpoints look good.404 route error, OpenAPI check, and 303 with Location validated.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/writer_kafka.py (2)
34-49: Validate SASL/SSL config keys before usePrevents KeyError at runtime. (Reiterating a prior suggestion.)
if "kafka_sasl_kerberos_principal" in CONFIG and "kafka_ssl_key_path" in CONFIG: - producer_config.update( + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in CONFIG] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": CONFIG["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": CONFIG["kafka_sasl_kerberos_principal"], "ssl.ca.location": CONFIG["kafka_ssl_ca_path"], "ssl.certificate.location": CONFIG["kafka_ssl_cert_path"], "ssl.key.location": CONFIG["kafka_ssl_key_path"], "ssl.key.password": CONFIG["kafka_ssl_key_password"], } )
72-75: Use logger.exception and drop blind exceptInclude traceback; satisfy TRY400/BLE001.
- except Exception as e: - msg = f"The Kafka writer failed with unknown error: {str(e)}" - _logger.error(msg) - return False, msg + except Exception: + _logger.exception("The Kafka writer failed with unknown error") + return False, "The Kafka writer failed with unknown error"src/event_gate_lambda.py (2)
167-175: Make Authorization parsing case-insensitive and robustHandles varied casings/spaces.
def extract_token(eventHeaders): # Initial implementation used bearer header directly if "bearer" in eventHeaders: return eventHeaders["bearer"] - - if "Authorization" in eventHeaders and eventHeaders["Authorization"].startswith("Bearer "): - return eventHeaders["Authorization"][len("Bearer ") :] - - return "" # Will result in 401 + auth = eventHeaders.get("Authorization") or eventHeaders.get("authorization") or "" + if isinstance(auth, str): + parts = auth.strip().split() + if len(parts) == 2 and parts[0].lower() == "bearer": + return parts[1] + return "" # Will result in 401
121-126: Catch PyJWT-specific errors instead of blind exceptAvoids masking unrelated exceptions and aligns with linters.
- try: - token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) - except Exception: + try: + token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) + except jwt.exceptions.InvalidTokenError: return _error_response(401, "auth", "Invalid or missing token")src/writer_postgres.py (3)
24-28: Remove unused noqa and tighten typing on psycopg2 fallback.Drop the unused noqa and use a targeted ignore on the assignment.
try: - import psycopg2 # noqa: F401 + import psycopg2 except ImportError: # pragma: no cover - environment without psycopg2 - psycopg2 = None # type: ignore + psycopg2 = None # type: ignore[assignment]
34-51: Type init() and harden secret fetch to avoid init-time crashes.Prevents failures when Secrets Manager access/shape is wrong; aligns with mypy goals.
-def init(logger): +def init(logger: logging.Logger) -> None: @@ - secret_name = os.environ.get("POSTGRES_SECRET_NAME", "") - secret_region = os.environ.get("POSTGRES_SECRET_REGION", "") + secret_name: str = os.environ.get("POSTGRES_SECRET_NAME", "") + secret_region: str = os.environ.get("POSTGRES_SECRET_REGION", "") @@ - if secret_name and secret_region: - aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) - postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] - POSTGRES = json.loads(postgres_secret) + if secret_name and secret_region: + try: + aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) + secret_value = aws_secrets.get_secret_value(SecretId=secret_name) + secret_str = secret_value.get("SecretString") + POSTGRES = json.loads(secret_str) if secret_str else {"database": ""} + except (ClientError, KeyError, json.JSONDecodeError): + _logger.exception("Failed to fetch/parse Postgres secret") + POSTGRES = {"database": ""}
239-242: Use logger.exception and fix message grammar (RUF010/TRY400/BLE001).Logs stack traces and addresses lint.
- except Exception as e: # pragma: no cover - defensive (still tested though) - err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - _logger.error(err_msg) + except Exception as e: # pragma: no cover - defensive (still tested though) + err_msg = f"The Postgres writer failed with unknown error: {e!s}" + _logger.exception(err_msg) return False, err_msg
🧹 Nitpick comments (10)
src/writer_eventbridge.py (2)
18-25: Add missing typing and annotate globals for mypyImport Any and type aws_eventbridge to avoid name-defined errors.
-from typing import Optional, Tuple +from typing import Any, Optional, Tuple @@ -aws_eventbridge = None # will hold boto3 client +aws_eventbridge: Any | None = None # will hold boto3 client
28-36: Type annotate init()Helps satisfy mypy and documents the API.
-def init(logger, CONFIG): +def init(logger: logging.Logger, CONFIG: dict[str, str]) -> None:src/writer_kafka.py (3)
65-65: Mark unused callback arg to satisfy lintersPrevents ARG005; keeps signature.
- callback=lambda err, msg: error.append(err) if err is not None else None, + callback=lambda err, _msg: error.append(err) if err else None,
67-67: Avoid indefinite flushBound the wait so lambda can return under broker issues.
- kafka_producer.flush() + kafka_producer.flush(timeout=10)
20-22: Remove unused boto3 importPylint will flag this.
-import boto3 from confluent_kafka import Producersrc/event_gate_lambda.py (1)
199-200: Log unexpected exceptions with tracebackUse logger.exception; avoid f-strings in logs.
- logger.error(f"Unexpected exception: {e}") + logger.exception("Unexpected exception")src/writer_postgres.py (4)
98-100: Avoid serializing None to the literal "null".When keys exist but are None, json.dumps produces "null" (string), not SQL NULL.
- json.dumps(message.get("format_options")) if "format_options" in message else None, - json.dumps(message.get("additional_info")) if "additional_info" in message else None, + (json.dumps(fo) if (fo := message.get("format_options")) is not None else None), + (json.dumps(ai) if (ai := message.get("additional_info")) is not None else None), @@ - json.dumps(job.get("additional_info")) if "additional_info" in job else None, + (json.dumps(ai) if (ai := job.get("additional_info")) is not None else None), @@ - json.dumps(message.get("additional_info")) if "additional_info" in message else None, + (json.dumps(ai) if (ai := message.get("additional_info")) is not None else None),Please confirm tests don’t rely on the "null" string; if they do, I can adjust with precomputed locals instead of the walrus operator.
Also applies to: 172-173, 205-206
53-101: Consider psycopg2.extras.Json for JSONB columns.If targets are json/jsonb columns, wrap dicts with Json(...) so PG sees JSON, not text.
Example (inside functions after psycopg2 availability check):
from psycopg2.extras import Json # type: ignore[import-not-found] payload = Json(message.get("additional_info")) if message.get("additional_info") is not None else NoneAlso applies to: 142-174, 177-207
238-238: Drop redundant commit inside connection context.The connection context manager commits on successful exit.
- connection.commit() # type: ignore + # commit handled by connection context manager
53-53: Add minimal type hints to helper signatures for mypy clarity.Improves readability and satisfies stricter configs.
-def postgres_edla_write(cursor, table, message): +def postgres_edla_write(cursor: Any, table: str, message: Dict[str, Any]) -> None: @@ -def postgres_run_write(cursor, table_runs, table_jobs, message): +def postgres_run_write(cursor: Any, table_runs: str, table_jobs: str, message: Dict[str, Any]) -> None: @@ -def postgres_test_write(cursor, table, message): +def postgres_test_write(cursor: Any, table: str, message: Dict[str, Any]) -> None:Also applies to: 104-104, 177-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pyproject.toml(1 hunks)src/event_gate_lambda.py(7 hunks)src/writer_eventbridge.py(2 hunks)src/writer_kafka.py(1 hunks)src/writer_postgres.py(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (4)
src/writer_eventbridge.py (2)
src/writer_kafka.py (2)
init(28-51)write(54-77)src/writer_postgres.py (2)
init(34-50)write(210-244)
src/writer_kafka.py (3)
src/writer_eventbridge.py (2)
init(28-37)write(40-69)src/writer_postgres.py (2)
init(34-50)write(210-244)tests/test_event_gate_lambda.py (2)
produce(30-33)flush(34-35)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (1)
read(60-65)src/writer_eventbridge.py (1)
init(28-37)src/writer_kafka.py (1)
init(28-51)src/writer_postgres.py (1)
init(34-50)
src/writer_postgres.py (3)
src/writer_eventbridge.py (2)
init(28-37)write(40-69)src/writer_kafka.py (2)
init(28-51)write(54-77)tests/test_writer_postgres.py (7)
client(208-209)cursor(152-153)execute(32-33)execute(141-142)connect(164-165)connect(195-196)commit(154-155)
🪛 Ruff (0.12.2)
src/writer_eventbridge.py
65-65: Use explicit conversion flag
Replace with conversion flag
(RUF010)
66-66: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer_kafka.py
65-65: Unused lambda argument: msg
(ARG005)
72-72: Do not catch blind exception: Exception
(BLE001)
73-73: Use explicit conversion flag
Replace with conversion flag
(RUF010)
74-74: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/event_gate_lambda.py
125-125: Do not catch blind exception: Exception
(BLE001)
src/writer_postgres.py
25-25: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
239-239: Do not catch blind exception: Exception
(BLE001)
240-240: Use explicit conversion flag
Replace with conversion flag
(RUF010)
241-241: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (6)
src/writer_eventbridge.py (2)
41-47: No-op guards look goodSkipping when config/client is absent prevents noisy failures in tests and local runs.
35-37: EventBridge EventBusName accepts ARN
AWS docs confirm PutEventsRequestEntry’s EventBusName field accepts both the short event bus name and the full ARN, so no change is needed.src/writer_kafka.py (1)
56-58: Graceful skip path LGTMReturning (True, None) when producer isn’t initialized is pragmatic.
src/event_gate_lambda.py (1)
140-151: Write() contract consistent: all writers return (bool, Optional[str]) and call-sites unpack correctly.src/writer_postgres.py (2)
55-86: Good move: parameterized queries.Switching from f-strings to parameterized placeholders eliminates SQL injection on values and improves correctness.
Also applies to: 106-140, 143-174, 180-207
53-54: Verified: all postgres_*_write calls use hardcoded table names; no further changes needed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
src/writer_eventbridge.py (2)
61-64: Log only failed entries from put_events and return concise error payloadDumping the whole response is noisy. Filter failed entries and return a compact message.
- if response.get("FailedEntryCount", 0) > 0: - msg = str(response) - logger.error(msg) - return False, msg + if response.get("FailedEntryCount", 0) > 0: + failures = [e for e in response.get("Entries", []) if "ErrorCode" in e or "ErrorMessage" in e] + logger.error("EventBridge put_events failed: %s", failures) + return False, json.dumps(failures)
65-76: Use logger.exception and avoid broad Exception rationale branchPrefer exception() to capture traceback; simplify unknown error handling.
- except (BotoCoreError, ClientError) as err: - err_msg = f"The EventBridge writer failed: {err}" # specific AWS error - logger.error(err_msg) - return False, err_msg - except Exception as err: # pragma: no cover - unexpected failure path - err_msg = ( - f"The EventBridge writer failed with unknown error: {err}" - if not isinstance(err, (BotoCoreError, ClientError)) - else str(err) - ) - logger.error(err_msg) - return False, err_msg + except (BotoCoreError, ClientError): + logger.exception("EventBridge put_events AWS error") + return False, "EventBridge put_events AWS error" + except Exception: # pragma: no cover - unexpected failure path + logger.exception("EventBridge put_events unexpected error") + return False, "EventBridge put_events unexpected error"src/writer_kafka.py (1)
25-38: Validate SASL/SSL keys before accessing to prevent runtime KeyErrorCheck presence of all referenced keys when enabling SASL_SSL.
- if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: - producer_config.update( + if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in config] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": config["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": config["kafka_sasl_kerberos_principal"], "ssl.ca.location": config["kafka_ssl_ca_path"], "ssl.certificate.location": config["kafka_ssl_cert_path"], "ssl.key.location": config["kafka_ssl_key_path"], "ssl.key.password": config["kafka_ssl_key_password"], } )src/event_gate_lambda.py (4)
200-205: Make Authorization parsing case-insensitive and robustHandle “authorization” key and “Bearer” scheme regardless of casing.
- auth_header = event_headers.get("Authorization", "") - if auth_header.startswith("Bearer "): - return auth_header[len("Bearer ") :] + auth = event_headers.get("Authorization") or event_headers.get("authorization") or "" + if isinstance(auth, str): + parts = auth.strip().split() + if len(parts) == 2 and parts[0].lower() == "bearer": + return parts[1] return ""
23-24: Remove TLS warning suppression and insecure verify=False patternEnable certificate verification; rely on system/AWS CA bundles.
-import urllib3 @@ -urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)Also applies to: 34-34
64-64: Do not disable TLS verification for boto3 S3 resourceRely on default verification (AWS_CA_BUNDLE supported).
-aws_s3 = boto3.Session().resource("s3", verify=False) # nosec Boto verify disabled intentionally +aws_s3 = boto3.Session().resource("s3")
79-81: Verify TLS for requests and check HTTP status before parsing JSONAvoid insecure verify=False and add raise_for_status.
-response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external -token_public_key_encoded = response_json["key"] -TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(base64.b64decode(token_public_key_encoded)) +resp = requests.get( + CONFIG["token_public_key_url"], + timeout=5, + verify=os.environ.get("REQUESTS_CA_BUNDLE", True), +) +resp.raise_for_status() +response_json = resp.json() +token_public_key_encoded = response_json["key"] +TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(base64.b64decode(token_public_key_encoded))src/writer_postgres.py (3)
14-16: Remove unused noqa on psycopg2 import (RUF100).The import is used later; the noqa is unnecessary.
- import psycopg2 # noqa: F401 + import psycopg2
263-266: Log unexpected failures with stack trace; fix grammar (TRY400/BLE001/RUF010).Switch to logger.exception and keep the “unknown error” wording for test expectations.
- except Exception as e: # pragma: no cover - defensive (still tested though) - err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + except Exception as err: # pragma: no cover - defensive (still tested though) + err_msg = f"The Postgres writer failed with unknown error: {err}" + _logger.exception(err_msg) + return False, err_msg
23-45: Harden AWS Secrets Manager fetch; avoid crashes and improve diagnostics.Wrap the secret retrieval/JSON parse and fall back cleanly. Also import AWS exceptions.
@@ -import boto3 +import boto3 +from botocore.exceptions import BotoCoreError, ClientError @@ - if secret_name and secret_region: - aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) - postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] - POSTGRES = json.loads(postgres_secret) + if secret_name and secret_region: + try: + aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) + secret_resp = aws_secrets.get_secret_value(SecretId=secret_name) + POSTGRES = json.loads(secret_resp["SecretString"]) + except (BotoCoreError, ClientError, KeyError, json.JSONDecodeError) as err: + _logger.exception("Failed to fetch/parse Postgres secret") + POSTGRES = {"database": ""} else: POSTGRES = {"database": ""}
🧹 Nitpick comments (7)
src/writer_eventbridge.py (2)
42-47: Skipping on missing ARN/client may hide misconfig — at least warnIf EventBridge is intended but misconfigured, a warning helps ops spot it.
- if not event_bus_arn: - logger.debug("No EventBus Arn - skipping") + if not event_bus_arn: + logger.warning("No EventBus ARN configured - skipping EventBridge publish") return True, None - if client is None: # defensive - logger.debug("EventBridge client not initialized - skipping") + if client is None: # defensive + logger.warning("EventBridge client not initialized - skipping") return True, None
50-50: Nit: capitalize “EventBridge” in logConsistency/readability.
- logger.debug("Sending to eventBridge %s", topic_name) + logger.debug("Sending to EventBridge %s", topic_name)src/writer_kafka.py (1)
62-73: Normalize callback errors to strings and log contextImproves error clarity and typing.
- error: list[Any] = [] + errors: list[str] = [] producer.produce( topic_name, key="", value=json.dumps(message).encode("utf-8"), - callback=lambda err, _msg: error.append(err) if err is not None else None, + callback=lambda err, _msg: errors.append(str(err)) if err is not None else None, ) producer.flush() - if error: - msg = str(error) - logger.error(msg) + if errors: + msg = "; ".join(errors) + logger.error("Kafka produce failed: %s", msg) return False, msgsrc/writer_postgres.py (4)
47-102: Use psycopg2.sql.Identifier for table names.Values are parameterized (good), but table names are f-string’d. They’re constants today, but using psycopg2.sql is safer and future-proof.
def postgres_edla_write(cursor, table: str, message: Dict[str, Any]) -> None: @@ - cursor.execute( - f""" - INSERT INTO {table} + from psycopg2 import sql # local import to keep optional dependency lazy + query = sql.SQL( + """ + INSERT INTO {} ( event_id, @@ - )""", - ( + )""" + ).format(sql.Identifier(table)) + + cursor.execute( + query, + ( message["event_id"], message["tenant_id"], message["source_app"], @@ - json.dumps(message.get("additional_info")) if "additional_info" in message else None, - ), + json.dumps(message.get("additional_info")) if "additional_info" in message else None, + ), )Apply the same pattern in postgres_run_write (both tables) and postgres_test_write.
105-149: OK on transaction semantics; consider minor polish.Single transaction for run header insert is fine. If you adopt psycopg2.sql for identifiers, mirror it here.
151-183: Validate jobs payload (avoid KeyError/TypeError).If message["jobs"] is missing/non-list, this raises. Either validate or guard.
- for job in message["jobs"]: + jobs = message.get("jobs") or [] + if not isinstance(jobs, list): + raise ValueError("message.jobs must be a list") + for job in jobs:
226-242: Guard against incomplete Postgres config (prevent KeyError on connect).Only checking “database” can still crash when host/user/password/port are missing.
- if not POSTGRES.get("database"): + if not POSTGRES.get("database"): _logger.debug("No Postgres - skipping") return True, None + required = ("host", "user", "password", "port") + if any(k not in POSTGRES or POSTGRES.get(k) in ("", None) for k in required): + _logger.debug("Incomplete Postgres config - skipping") + return True, None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/event_gate_lambda.py(3 hunks)src/writer_eventbridge.py(1 hunks)src/writer_kafka.py(1 hunks)src/writer_postgres.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/writer_eventbridge.py (2)
src/writer_kafka.py (2)
init(15-42)write(45-79)src/writer_postgres.py (2)
init(23-44)write(226-268)
src/writer_kafka.py (2)
src/writer_eventbridge.py (2)
init(16-26)write(29-78)tests/test_event_gate_lambda.py (3)
get(67-68)produce(30-33)flush(34-35)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (4)
get(67-68)read(60-65)Bucket(73-74)Object(70-71)src/writer_eventbridge.py (2)
init(16-26)write(29-78)src/writer_kafka.py (2)
init(15-42)write(45-79)src/writer_postgres.py (2)
init(23-44)write(226-268)
src/writer_postgres.py (3)
src/writer_eventbridge.py (2)
init(16-26)write(29-78)src/writer_kafka.py (2)
init(15-42)write(45-79)tests/test_writer_postgres.py (7)
client(208-209)cursor(152-153)execute(32-33)execute(141-142)connect(164-165)connect(195-196)commit(154-155)
🪛 Ruff (0.12.2)
src/writer_eventbridge.py
67-67: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
69-69: Do not catch blind exception: Exception
(BLE001)
75-75: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer_kafka.py
74-74: Do not catch blind exception: Exception
(BLE001)
76-76: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/event_gate_lambda.py
79-79: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
208-208: Unused function argument: context
(ARG001)
234-234: Do not catch blind exception: Exception
(BLE001)
235-235: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer_postgres.py
14-14: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
263-263: Do not catch blind exception: Exception
(BLE001)
264-264: Use explicit conversion flag
Replace with conversion flag
(RUF010)
265-265: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (4)
src/writer_postgres.py (4)
1-4: Good module scaffold and docstring.Clear intent, helps readers and tooling.
18-21: Typed globals look good.Sane defaults and logger setup aid mypy and safe pre-init usage.
186-223: Confirm timestamp field name consistency.This uses message["timestamp"] while EDLA uses message["timestamp_event"]. Ensure the producer/tests align.
243-263: Commit inside context is fine; keep if tests expect it.psycopg2 connection context usually commits on successful exit; explicit commit is harmless and keeps tests deterministic.
- initial setup of QA tools - applied Black
…ge writer for improved readability and maintainability Refactor event_gate_lambda and writer modules for improved type hinting, logging, and error handling; enhance readability and maintainability Refactor event writers to improve type hinting and logging; enhance error handling in write methods Add copyright headers and licensing information to source files Enhance error handling and logging in writers; refactor tests to use pytest Refactor code for consistency and readability; update requirements for testing and dependencies
oto-macenauer-absa
force-pushed
the
feature/54-Add-QA-tooling
branch
from
59f2ddd to
fca8811
Compare
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/writer_kafka.py (2)
58-81: Bug: usingkafka_producerinstead ofproducercauses mypy errors and runtimeNoneaccess.Also mix of
_loggervslogger. Use theSTATE-held producer and the locallogger.- logger = STATE["logger"] - producer: Optional[Producer] = STATE.get("producer") # type: ignore[assignment] + logger = STATE["logger"] + producer: Optional[Producer] = STATE.get("producer") # type: ignore[assignment] @@ - _logger.debug(f"Sending to kafka {topic_name}") - errors: list[Any] = [] - kafka_producer.produce( + logger.debug("Sending to Kafka %s", topic_name) + errors: list[Any] = [] + producer.produce( topic_name, key="", value=json.dumps(message).encode("utf-8"), - callback=lambda err, msg: (errors.append(str(err)) if err is not None else None), + callback=lambda err, _msg: (errors.append(str(err)) if err is not None else None), ) - kafka_producer.flush() + producer.flush() if errors: msg = "; ".join(errors) - _logger.error(msg) + logger.error(msg) return False, msg - except Exception as e: - err_msg = f"The Kafka writer failed with unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + except Exception as e: + err_msg = f"The Kafka writer failed with unknown error: {e}" + logger.error(err_msg) + return False, err_msg
1-84: Replace remainingkafka_producerreferences withproducerand re-run mypy
- In src/writer_kafka.py at lines 67 and 73, change
kafka_producer.produce(→producer.produce(andkafka_producer.flush()→producer.flush()- After updating, run mypy on this file to confirm no type errors remain.
src/event_gate_lambda.py (1)
32-36: Fix mypy “no-redef” (CONF_DIR/INVALID_CONF_ENV) by aliasing imports.
mypy flags duplicate names across try/except imports. Alias once and drop the extra rebind block.-try: - from .conf_path import CONF_DIR, INVALID_CONF_ENV -except ImportError: # fallback when executed outside package context - from conf_path import CONF_DIR, INVALID_CONF_ENV - -# Use imported symbols for internal variables -_CONF_DIR = CONF_DIR -_INVALID_CONF_ENV = INVALID_CONF_ENV +try: + from .conf_path import CONF_DIR as _CONF_DIR, INVALID_CONF_ENV as _INVALID_CONF_ENV +except ImportError: # fallback when executed outside package context + from conf_path import CONF_DIR as _CONF_DIR, INVALID_CONF_ENV as _INVALID_CONF_ENVAlso applies to: 37-40
♻️ Duplicate comments (11)
DEVELOPER.md (2)
15-18: Repo directory case fixed — thank you.
76-99: All “mypy” references spelled correctly now.tests/test_conf_validation.py (1)
62-65: Parametrization collects 0 tests when no topic_*.json exist — add an explicit existence test.The in-test assert won’t run if the param list is empty.
Add alongside existing tests:
def test_topic_json_files_exist(): files = glob(os.path.join(CONF_DIR, "topic_*.json")) assert files, "No topic_*.json files found"src/writer_kafka.py (1)
28-46: Validate required config and SASL/SSL keys to avoid KeyError at runtime.- producer_config: Dict[str, Any] = {"bootstrap.servers": config["kafka_bootstrap_server"]} + if "kafka_bootstrap_server" not in config: + raise KeyError("Missing 'kafka_bootstrap_server' in config") + producer_config: Dict[str, Any] = {"bootstrap.servers": config["kafka_bootstrap_server"]} if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: - producer_config.update( + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in config] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( {src/writer_postgres.py (2)
37-44: Handle Secrets Manager errors explicitly and fall back safely.- if secret_name and secret_region: - aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) - postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] - POSTGRES = json.loads(postgres_secret) + if secret_name and secret_region: + try: + aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) + postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] + POSTGRES = json.loads(postgres_secret) + except Exception: + _logger.exception("Failed to fetch/parse Postgres secret — falling back to empty config") + POSTGRES = {"database": ""}
263-266: Uselogger.exceptionand fix message grammar.-except Exception as e: # pragma: no cover - defensive (still tested though) - err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg +except Exception as e: # pragma: no cover - defensive (still tested though) + _logger.exception("The Postgres writer failed with unknown error") + return False, f"The Postgres writer failed with unknown error: {e}"src/event_gate_lambda.py (2)
271-273: Log unexpected exceptions with traceback.
Improves diagnosability and satisfies TRY400.- except Exception as exc: # pylint: disable=broad-exception-caught - logger.error("Unexpected exception: %s", exc) + except Exception: # pylint: disable=broad-exception-caught + logger.exception("Unexpected exception") return _error_response(500, "internal", "Unexpected server error")
23-24: Remove verify=False and TLS warning suppression; keep TLS verification on.
Security issue and already raised previously. Use system CA (default) or env-provided CA bundle; keep timeouts.-import urllib3 @@ -urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) @@ -aws_s3 = boto3.Session().resource("s3", verify=False) # nosec Boto verify disabled intentionally +aws_s3 = boto3.Session().resource("s3") @@ -response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external +ca_bundle = os.environ.get("REQUESTS_CA_BUNDLE") # optional custom CA +response_json = requests.get( + CONFIG["token_public_key_url"], + timeout=10, + verify=ca_bundle if ca_bundle else True, +).json()Also applies to: 43-43, 76-78, 91-94
tests/test_event_gate_lambda.py (3)
25-42: Use non-clobbering module stubs (respect installed packages).
Check import availability with importlib before inserting dummies; same pattern for confluent_kafka.-# Inject dummy confluent_kafka if not installed so patching works -if "confluent_kafka" not in sys.modules: +# Inject dummy confluent_kafka if not installed so patching works +import importlib.util +if importlib.util.find_spec("confluent_kafka") is None: dummy_ck = types.ModuleType("confluent_kafka") @@ - sys.modules["confluent_kafka"] = dummy_ck + sys.modules.setdefault("confluent_kafka", dummy_ck)
43-46: Apply the same non-clobbering pattern for psycopg2.
Prevents masking a real driver.-# Inject dummy psycopg2 (optional dependency) -if "psycopg2" not in sys.modules: - sys.modules["psycopg2"] = types.ModuleType("psycopg2") +# Inject dummy psycopg2 (optional dependency) +if importlib.util.find_spec("psycopg2") is None: + sys.modules.setdefault("psycopg2", types.ModuleType("psycopg2"))
102-105: Don’t call patch.stopall() in a shared fixture.
It can interfere with other tests; the explicit loop is enough.for p in started_patches: p.stop() - patch.stopall()
🧹 Nitpick comments (17)
DEVELOPER.md (3)
71-74: Specify fenced-code language for the “Expected Output” block.Fixes MD040. Use a neutral lexer.
-``` +```text All done! ✨ 🍰 ✨ 1 file reformatted.--- `9-9`: **TOC link “Run Action Locally” has no matching section.** Either add the section or drop the TOC entry. ```diff -- [Run Action Locally](#run-action-locally)
127-128: Remove stray heading marker.Dangling “##” at EOF.
-## -tests/test_conf_validation.py (1)
34-35: Open JSON with explicit UTF-8.Prevents locale-dependent parses.
-def load_json(path): - with open(path, "r") as f: +def load_json(path): + with open(path, "r", encoding="utf-8") as f: return json.load(f)src/writer_eventbridge.py (2)
48-53: Treat missing EventBus ARN as a configuration issue (warn).Returning success is fine for “skip” semantics, but log a warning to aid ops.
- if not event_bus_arn: - logger.debug("No EventBus Arn - skipping") + if not event_bus_arn: + logger.warning("No EventBridge EventBus ARN configured - skipping") return True, None
24-26: Optional: set client timeouts/retries via botocore Config.Improves resilience to transient AWS issues.
# example: from botocore.config import Config STATE["client"] = boto3.client("events", config=Config(connect_timeout=3, read_timeout=3, retries={"max_attempts": 3}))src/writer_kafka.py (1)
10-11: CatchKafkaExceptionexplicitly and keep traceback vialogger.exception.-from confluent_kafka import Producer +from confluent_kafka import Producer, KafkaException @@ - except Exception as e: - err_msg = f"The Kafka writer failed with unknown error: {str(e)}" - logger.error(err_msg) - return False, err_msg + except KafkaException as e: # pragma: no cover + logger.exception("Kafka producer error") + return False, str(e) + except Exception: # pragma: no cover + logger.exception("Kafka writer unexpected error") + return False, "Kafka writer unexpected error"Also applies to: 78-81
src/writer_postgres.py (2)
13-17: Remove unused# noqa: F401on psycopg2 import.Ruff flags RUF100; import is used later.
-try: - import psycopg2 # noqa: F401 +try: + import psycopg2 except ImportError: # pragma: no cover - environment without psycopg2 psycopg2 = None # type: ignore
151-183: Guard iteration over jobs.Defensive in case payload lacks jobs.
- for job in message["jobs"]: + for job in message.get("jobs", []):tests/test_writer_postgres.py (4)
145-154: Remove unused fixture parameter to satisfy linters.
reset_statedoesn’t usemonkeypatch; drop it or underscore it.-@pytest.fixture -def reset_state(monkeypatch): +@pytest.fixture +def reset_state():
192-194: Silence ARG002 by underscoring unused kwargs in test doubles.
Purely cosmetic; improves Ruff signal.class DummyPsycopg: def __init__(self, store): self.store = store - def connect(self, **kwargs): + def connect(self, **_kwargs): return DummyConnection(self.store) class FailingPsycopg: - def connect(self, **kwargs): + def connect(self, **_kwargs): raise RuntimeError("boom")Also applies to: 228-230
239-241: Avoid Bandit S105 false positive in tests by using neutral names.
Rename the illustrative secret name to a neutral placeholder.- os.environ["POSTGRES_SECRET_NAME"] = "mysecret" + os.environ["POSTGRES_SECRET_NAME"] = "dummy-secret-name"
59-66: Ruff S101 on pytest asserts: configure per-file ignore instead of changing tests.
Tests should use bareassert. Add this to pyproject/ruff config.[tool.ruff.lint.per-file-ignores] "tests/**" = ["S101"]src/event_gate_lambda.py (2)
245-246: Underscore unused Lambdacontextarg to quiet linters.
Pylint is disabled, but Ruff still warns; trivial rename.-def lambda_handler(event: Dict[str, Any], context: Any): # pylint: disable=unused-argument,too-many-return-statements +def lambda_handler(event: Dict[str, Any], _context: Any): # pylint: disable=unused-argument,too-many-return-statements
262-267: Return 400 for invalid JSON request bodies.
Currently a JSON parse error falls into the 500 handler. Handle it explicitly.- if method == "POST": - return post_topic_message( - event["pathParameters"]["topic_name"].lower(), - json.loads(event["body"]), - extract_token(event.get("headers", {})), - ) + if method == "POST": + try: + body_obj = json.loads(event["body"]) + except Exception: + return _error_response(400, "validation", "Invalid JSON body") + return post_topic_message( + event["pathParameters"]["topic_name"].lower(), + body_obj, + extract_token(event.get("headers", {})), + )tests/test_event_gate_lambda.py (2)
29-33: Underscore unused args in dummy Producer to satisfy Ruff.
Cosmetic; keeps tests green with strict lint.- class DummyProducer: # minimal interface - def __init__(self, *a, **kw): + class DummyProducer: # minimal interface + def __init__(self, *_a, **_kw): pass @@ - def produce(self, *a, **kw): - cb = kw.get("callback") + def produce(self, *_a, **_kw): + cb = _kw.get("callback")
129-309: Ruff S101 on pytest asserts: prefer config over code churn.
Permit bareassertin tests via per-file ignore.[tool.ruff.lint.per-file-ignores] "tests/**" = ["S101"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/CODEOWNERS(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).pylintrc(1 hunks)DEVELOPER.md(1 hunks)README.md(1 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)src/event_gate_lambda.py(5 hunks)src/requirements.txt(0 hunks)src/writer_eventbridge.py(1 hunks)src/writer_kafka.py(1 hunks)src/writer_postgres.py(4 hunks)tests/conftest.py(1 hunks)tests/test_conf_validation.py(2 hunks)tests/test_event_gate_lambda.py(1 hunks)tests/test_writer_postgres.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/requirements.txt
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- .github/CODEOWNERS
- tests/conftest.py
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .pylintrc
- .github/workflows/test.yml
- requirements.txt
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (6)
tests/test_writer_postgres.py (1)
src/writer_postgres.py (5)
init(23-44)postgres_edla_write(47-102)postgres_run_write(105-183)postgres_test_write(186-223)write(226-268)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (4)
get(75-76)read(65-72)Bucket(83-84)Object(79-80)src/writer_eventbridge.py (2)
init(16-26)write(35-79)src/writer_kafka.py (2)
init(19-46)write(49-83)src/writer_postgres.py (2)
init(23-44)write(226-268)
tests/test_event_gate_lambda.py (1)
src/event_gate_lambda.py (1)
lambda_handler(245-273)
src/writer_postgres.py (3)
src/writer_eventbridge.py (2)
init(16-26)write(35-79)src/writer_kafka.py (2)
init(19-46)write(49-83)tests/test_writer_postgres.py (5)
client(244-245)cursor(175-176)connect(192-193)connect(228-229)commit(178-179)
src/writer_eventbridge.py (1)
src/writer_kafka.py (2)
init(19-46)write(49-83)
src/writer_kafka.py (3)
src/writer_eventbridge.py (2)
init(16-26)write(35-79)src/writer_postgres.py (2)
init(23-44)write(226-268)tests/test_event_gate_lambda.py (2)
get(75-76)produce(32-35)
🪛 Ruff (0.12.2)
tests/test_writer_postgres.py
59-59: Use of assert detected
(S101)
61-61: Use of assert detected
(S101)
62-62: Use of assert detected
(S101)
63-63: Use of assert detected
(S101)
64-64: Use of assert detected
(S101)
65-65: Use of assert detected
(S101)
66-66: Use of assert detected
(S101)
84-84: Use of assert detected
(S101)
85-85: Use of assert detected
(S101)
86-86: Use of assert detected
(S101)
87-87: Use of assert detected
(S101)
114-114: Use of assert detected
(S101)
116-116: Use of assert detected
(S101)
117-117: Use of assert detected
(S101)
119-119: Use of assert detected
(S101)
120-120: Use of assert detected
(S101)
134-134: Use of assert detected
(S101)
136-136: Use of assert detected
(S101)
137-137: Use of assert detected
(S101)
138-138: Use of assert detected
(S101)
145-145: Unused function argument: monkeypatch
(ARG001)
192-192: Unused method argument: kwargs
(ARG002)
196-196: Unused function argument: reset_state
(ARG001)
199-199: Use of assert detected
(S101)
202-202: Unused function argument: reset_state
(ARG001)
206-206: Use of assert detected
(S101)
209-209: Unused function argument: reset_state
(ARG001)
214-214: Use of assert detected
(S101)
217-217: Unused function argument: reset_state
(ARG001)
223-223: Use of assert detected
(S101)
226-226: Unused function argument: reset_state
(ARG001)
228-228: Unused method argument: kwargs
(ARG002)
234-234: Use of assert detected
(S101)
237-237: Unused function argument: reset_state
(ARG001)
239-239: Possible hardcoded password assigned to: "POSTGRES_SECRET_NAME"
(S105)
240-240: Possible hardcoded password assigned to: "POSTGRES_SECRET_REGION"
(S105)
241-241: Unused lambda argument: SecretId
(ARG005)
244-244: Unused method argument: service_name
(ARG002)
244-244: Unused method argument: region_name
(ARG002)
249-249: Use of assert detected
(S101)
252-252: Unused function argument: reset_state
(ARG001)
268-268: Use of assert detected
(S101)
271-271: Unused function argument: reset_state
(ARG001)
287-287: Use of assert detected
(S101)
src/event_gate_lambda.py
91-91: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
245-245: Unused function argument: context
(ARG001)
271-271: Do not catch blind exception: Exception
(BLE001)
272-272: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_event_gate_lambda.py
32-32: Unused method argument: a
(ARG002)
79-79: Unused method argument: key
(ARG002)
83-83: Unused method argument: name
(ARG002)
132-132: Use of assert detected
(S101)
134-134: Use of assert detected
(S101)
140-140: Use of assert detected
(S101)
142-142: Use of assert detected
(S101)
148-148: Use of assert detected
(S101)
159-159: Use of assert detected
(S101)
161-161: Use of assert detected
(S101)
162-162: Use of assert detected
(S101)
175-175: Use of assert detected
(S101)
177-177: Use of assert detected
(S101)
191-191: Use of assert detected
(S101)
193-193: Use of assert detected
(S101)
214-214: Use of assert detected
(S101)
216-216: Use of assert detected
(S101)
217-217: Use of assert detected
(S101)
235-235: Use of assert detected
(S101)
237-237: Use of assert detected
(S101)
238-238: Use of assert detected
(S101)
239-239: Use of assert detected
(S101)
257-257: Use of assert detected
(S101)
259-259: Use of assert detected
(S101)
277-277: Use of assert detected
(S101)
283-283: Use of assert detected
(S101)
285-285: Use of assert detected
(S101)
291-291: Use of assert detected
(S101)
292-292: Use of assert detected
(S101)
298-298: Use of assert detected
(S101)
299-299: Use of assert detected
(S101)
306-306: Use of assert detected
(S101)
308-308: Use of assert detected
(S101)
src/writer_postgres.py
14-14: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
263-263: Do not catch blind exception: Exception
(BLE001)
src/writer_kafka.py
71-71: Unused lambda argument: msg
(ARG005)
tests/test_conf_validation.py
41-41: Use of assert detected
(S101)
49-49: Use of assert detected
(S101)
55-55: Use of assert detected
(S101)
57-57: Use of assert detected
(S101)
58-58: Use of assert detected
(S101)
59-59: Use of assert detected
(S101)
64-64: Use of assert detected
(S101)
66-66: Use of assert detected
(S101)
68-68: Use of assert detected
(S101)
70-70: Use of assert detected
(S101)
72-72: Use of assert detected
(S101)
74-74: Use of assert detected
(S101)
75-75: Use of assert detected
(S101)
🪛 GitHub Actions: Build and Test
src/event_gate_lambda.py
[error] 35-35: src/event_gate_lambda.py:35: Name 'CONF_DIR' already defined (possibly by an import) [no-redef]. Command: mypy .
[error] 35-35: src/event_gate_lambda.py:35: Name 'INVALID_CONF_ENV' already defined (possibly by an import) [no-redef]. Command: mypy .
src/writer_kafka.py
[error] 67-67: src/writer_kafka.py:67: Item 'None' of 'Any | None' has no attribute 'produce' [union-attr]. Command: mypy .
[error] 73-73: src/writer_kafka.py:73: Item 'None' of 'Any | None' has no attribute 'flush' [union-attr]. Command: mypy .
🪛 markdownlint-cli2 (0.17.2)
DEVELOPER.md
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
src/writer_eventbridge.py (1)
35-79: Overall: solid, typed, and precise error handling.Consistent API and failure formatting look good.
src/writer_kafka.py (1)
67-73: Ruff ARG005: mark unused lambda arg.Addressed in main fix by renaming to
_msg.src/writer_postgres.py (1)
55-102: Nice move to parameterized queries — safer and clearer.Also applies to: 114-149, 194-223
tests/test_writer_postgres.py (1)
26-29: LGTM: module logger init is clear and keeps tests quiet.
✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/writer_kafka.py (1)
39-51: Validate all SASL/SSL keys before updating configCurrent code checks two flags but indexes several keys; missing ones raise KeyError at runtime. This was flagged earlier; looks reintroduced.
- if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: - producer_config.update( + if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in config or not config.get(k)] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": config["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": config["kafka_sasl_kerberos_principal"], "ssl.ca.location": config["kafka_ssl_ca_path"], "ssl.certificate.location": config["kafka_ssl_cert_path"], "ssl.key.location": config["kafka_ssl_key_path"], "ssl.key.password": config["kafka_ssl_key_password"], } )src/event_gate_lambda.py (3)
47-47: Do not disable TLS warningsRemoving certificate warnings hides real issues and violates secure-by-default.
-urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)Also remove the now-unused
import urllib3.-import urllib3
76-76: Re-enable TLS verification and add proper timeouts
verify=Falseon boto3/requests is insecure and flagged by linters. Use defaults (verification on) and keep timeouts.-aws_s3 = boto3.Session().resource("s3", verify=False) # nosec Boto verify disabled intentionally +aws_s3 = boto3.Session().resource("s3")-response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external +response_json = requests.get(CONFIG["token_public_key_url"], timeout=5).json()If a custom CA is needed, pass
verify=os.environ.get("REQUESTS_CA_BUNDLE").Also applies to: 91-93
271-273: Log unexpected exceptions with tracebackPrefer
logger.exceptionto retain stack traces.- except Exception as exc: # pylint: disable=broad-exception-caught - logger.error("Unexpected exception: %s", exc) + except Exception: # pylint: disable=broad-exception-caught + logger.exception("Unexpected exception") return _error_response(500, "internal", "Unexpected server error")tests/test_event_gate_lambda.py (1)
24-46: Don’t clobber real installs; inject dummy modules inside the fixture with find_spec + patch.dict.Using “if name not in sys.modules” can mask an actually installed package that hasn’t been imported yet. Localize these stubs to the fixture and only when truly missing. Also restores state cleanly. Note: the psycopg2 part repeats a previously raised comment.
Apply these diffs:
@@ -import importlib +import importlib +import importlib.util @@ -# Inject dummy confluent_kafka if not installed so patching works -if "confluent_kafka" not in sys.modules: - dummy_ck = types.ModuleType("confluent_kafka") - - class DummyProducer: # minimal interface - def __init__(self, *a, **kw): - pass - - def produce(self, *a, **kw): - cb = kw.get("callback") - if cb: - cb(None, None) - - def flush(self): - return None - - dummy_ck.Producer = DummyProducer - sys.modules["confluent_kafka"] = dummy_ck - -# Inject dummy psycopg2 (optional dependency) -if "psycopg2" not in sys.modules: - sys.modules["psycopg2"] = types.ModuleType("psycopg2") +# (moved into fixture to avoid global side effects)@@ @pytest.fixture(scope="module") def event_gate_module(): - started_patches = [] + started_patches = [] + + # Safely inject optional deps only when truly missing; auto-restore on teardown. + modules_to_inject = {} + if importlib.util.find_spec("confluent_kafka") is None: + dummy_ck = types.ModuleType("confluent_kafka") + + class DummyProducer: # minimal interface + def __init__(self, *_args, **_kwargs): + pass + + def produce(self, *_args, **_kwargs): + cb = _kwargs.get("callback") + if cb: + cb(None, None) + + def flush(self): + return None + + dummy_ck.Producer = DummyProducer + modules_to_inject["confluent_kafka"] = dummy_ck + if importlib.util.find_spec("psycopg2") is None: + modules_to_inject["psycopg2"] = types.ModuleType("psycopg2") + + ctx = patch.dict(sys.modules, modules_to_inject, clear=False) + ctx.start() + started_patches.append(ctx)Also applies to: 48-56
🧹 Nitpick comments (14)
tests/test_conf_path.py (4)
22-29: Make assertion resilient to repo renamesHard-coding endswith("EventGate/conf") is brittle. Compute expected path from the module instead.
- # Should fall back to repository conf directory - assert conf_dir.endswith(os.path.join("EventGate", "conf")) + # Should fall back to repository conf directory + expected = (Path(conf_path_module.__file__).resolve().parent.parent / "conf").resolve() + assert Path(conf_dir).resolve() == expected
31-50: Tighten helper: remove unused import and clean sys.modulesMinor hygiene to avoid leaks across tests.
- import inspect code = Path(conf_path_module.__file__).read_text(encoding="utf-8") @@ - sys.modules[spec.name] = mod # ensure import works for dependencies + sys.modules[spec.name] = mod # ensure import works for dependencies spec.loader.exec_module(mod) # type: ignore[attr-defined] @@ - return mod + # Provide cleanup hook to drop sys.modules entry too + def _cleanup(): + tmp.cleanup() + sys.modules.pop(spec.name, None) + mod._tmp = type("T", (), {"cleanup": _cleanup})() # type: ignore[attr-defined] + return mod
1-129: Fix style gate: Black reformatting requiredCI fails on Black. Please run the project’s formatter (e.g.,
black .) to align formatting.
1-129: Ruff S101 in testsIf Ruff enforces S101 globally, prefer suppressing it for tests instead of replacing pytest asserts.
Add to pyproject.toml:
[tool.ruff.lint.per-file-ignores] "tests/**/*.py" = ["S101"]src/writer_kafka.py (3)
82-82: Silence ARG005: mark unused callback argName the unused
msgargument_msg.- callback=lambda err, msg: (errors.append(str(err)) if err is not None else None), + callback=lambda err, _msg: (errors.append(str(err)) if err is not None else None),
85-88: Return clearer error and keep traceback with logger.exceptionMessage says “unknown error” while catching KafkaException; use a stable log line and return the exception string.
- except KafkaException as e: # narrow exception capture - err_msg = f"The Kafka writer failed with unknown error: {str(e)}" - _logger.exception(err_msg) - return False, err_msg + except KafkaException as e: # narrow exception capture + logger.exception("Kafka producer error") + return False, str(e)
1-96: Fix style gate: Black reformatting requiredCI flags Black reformatting. Please run the formatter.
src/event_gate_lambda.py (3)
245-246: Silence unused-argument warning for contextRename to
_contextto satisfy linters.-def lambda_handler(event: Dict[str, Any], context: Any): # pylint: disable=unused-argument,too-many-return-statements +def lambda_handler(event: Dict[str, Any], _context: Any): # pylint: disable=unused-argument,too-many-return-statements
89-95: Avoid network I/O at import-time for public keyFetching the key during import can break cold starts and unit tests; lazily load/cache on first use instead.
I can provide a small loader that caches the key the first time
post_topic_messageruns. Want me to draft it?
1-206: Optional: add Content-Type for /apiConsider returning
Content-Type: text/yamlfor the OpenAPI body.def get_api() -> Dict[str, Any]: """Return the OpenAPI specification text.""" - return {"statusCode": 200, "body": API} + return {"statusCode": 200, "headers": {"Content-Type": "text/yaml"}, "body": API}tests/test_event_gate_lambda.py (4)
52-56: Prefer autospec=True in helper to catch signature drift early.This tightens mocks to real call signatures and reduces false positives.
- def start_patch(target): - p = patch(target) + def start_patch(target, **kwargs): + p = patch(target, autospec=True, **kwargs) started_patches.append(p) return p.start()
28-33: Silence “unused args” in DummyProducer by using underscored var names.Keeps linters quiet without changing behavior.
- class DummyProducer: # minimal interface - def __init__(self, *a, **kw): + class DummyProducer: # minimal interface + def __init__(self, *_args, **_kwargs): pass - - def produce(self, *a, **kw): - cb = kw.get("callback") + def produce(self, *_args, **_kwargs): + cb = _kwargs.get("callback") if cb: cb(None, None)
79-85: Underscore unused parameters in S3 stubs.Minor lint cleanup.
class MockS3Bucket: - def Object(self, key): + def Object(self, _key): return MockS3Object() class MockS3Resource: - def Bucket(self, name): + def Bucket(self, _name): return MockS3Bucket()
128-307: Use bare asserts in tests; if Bandit/Ruff are enabled, ignore S101 for tests.Pytest’s bare asserts are idiomatic. If you wire up Ruff/flake8-bandit, prefer a per-file ignore rather than rewriting tests.
Add to pyproject.toml:
[tool.ruff.lint.per-file-ignores] "tests/**" = ["S101", "ARG002"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
requirements.txt(1 hunks)src/event_gate_lambda.py(5 hunks)src/writer_kafka.py(1 hunks)tests/test_conf_path.py(1 hunks)tests/test_event_gate_lambda.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_conf_path.py (1)
src/conf_path.py (1)
resolve_conf_dir(26-54)
tests/test_event_gate_lambda.py (1)
src/event_gate_lambda.py (1)
lambda_handler(245-273)
src/writer_kafka.py (2)
src/writer_eventbridge.py (2)
init(16-26)write(35-79)tests/test_event_gate_lambda.py (3)
get(75-76)produce(32-35)flush(37-38)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (4)
get(75-76)read(65-72)Bucket(83-84)Object(79-80)src/writer_kafka.py (2)
init(23-56)write(59-95)src/writer_eventbridge.py (2)
init(16-26)write(35-79)src/writer_postgres.py (2)
init(23-44)write(226-268)
🪛 Ruff (0.12.2)
tests/test_conf_path.py
18-18: Use of assert detected
(S101)
19-19: Use of assert detected
(S101)
27-27: Use of assert detected
(S101)
28-28: Use of assert detected
(S101)
43-43: Use of assert detected
(S101)
66-66: Use of assert detected
(S101)
67-67: Use of assert detected
(S101)
85-85: Use of assert detected
(S101)
86-86: Use of assert detected
(S101)
105-105: Use of assert detected
(S101)
106-106: Use of assert detected
(S101)
125-125: Use of assert detected
(S101)
126-126: Use of assert detected
(S101)
tests/test_event_gate_lambda.py
32-32: Unused method argument: a
(ARG002)
79-79: Unused method argument: key
(ARG002)
83-83: Unused method argument: name
(ARG002)
131-131: Use of assert detected
(S101)
133-133: Use of assert detected
(S101)
139-139: Use of assert detected
(S101)
141-141: Use of assert detected
(S101)
147-147: Use of assert detected
(S101)
158-158: Use of assert detected
(S101)
160-160: Use of assert detected
(S101)
161-161: Use of assert detected
(S101)
174-174: Use of assert detected
(S101)
176-176: Use of assert detected
(S101)
190-190: Use of assert detected
(S101)
192-192: Use of assert detected
(S101)
213-213: Use of assert detected
(S101)
215-215: Use of assert detected
(S101)
216-216: Use of assert detected
(S101)
234-234: Use of assert detected
(S101)
236-236: Use of assert detected
(S101)
237-237: Use of assert detected
(S101)
238-238: Use of assert detected
(S101)
256-256: Use of assert detected
(S101)
258-258: Use of assert detected
(S101)
276-276: Use of assert detected
(S101)
282-282: Use of assert detected
(S101)
284-284: Use of assert detected
(S101)
290-290: Use of assert detected
(S101)
291-291: Use of assert detected
(S101)
297-297: Use of assert detected
(S101)
298-298: Use of assert detected
(S101)
305-305: Use of assert detected
(S101)
307-307: Use of assert detected
(S101)
src/writer_kafka.py
13-13: Do not catch blind exception: Exception
(BLE001)
35-35: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Unused lambda argument: msg
(ARG005)
86-86: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/event_gate_lambda.py
91-91: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
245-245: Unused function argument: context
(ARG001)
271-271: Do not catch blind exception: Exception
(BLE001)
272-272: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 GitHub Actions: Build and Test
tests/test_conf_path.py
[error] 1-1: Black formatting would reformat this file. Command: black --check $(git ls-files '*.py')
src/writer_kafka.py
[error] 1-1: Black formatting would reformat this file. Command: black --check $(git ls-files '*.py')
🔇 Additional comments (6)
tests/test_conf_path.py (5)
12-20: LGTM: env var selects a valid directoryCovers the happy path precisely.
53-70: LGTM: prefers current_dir/conf when parent missingMatches resolve_conf_dir’s fallback order.
72-89: LGTM: returns parent conf path even if absentAs designed by resolve_conf_dir.
91-109: LGTM: invalid env + current conf available → current conf winsAlso validates recording of the invalid env path.
111-129: LGTM: invalid env + no conf anywhere → parent path returnedCovers final fallback and invalid path reporting.
src/event_gate_lambda.py (1)
1-274: Insecure TLS verify=False occurrences
Found only two instances ofverify=False:
- src/event_gate_lambda.py:76 (boto3.Session().resource call)
- src/event_gate_lambda.py:91 (requests.get call)
No otherverify=Falseusages detected.
…d event gate lambda
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/writer_kafka.py (1)
39-51: Validate all SASL/SSL keys before accessing them (avoid KeyError).You gate on two keys but index six others unconditionally. Validate and fail fast with a clear error.
- if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: - producer_config.update( + if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in config or not config.get(k)] + if missing: + raise ValueError(f"Missing Kafka SASL/SSL config keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": config["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": config["kafka_sasl_kerberos_principal"], "ssl.ca.location": config["kafka_ssl_ca_path"], "ssl.certificate.location": config["kafka_ssl_cert_path"], "ssl.key.location": config["kafka_ssl_key_path"], "ssl.key.password": config["kafka_ssl_key_password"], } )src/event_gate_lambda.py (2)
268-270: Log unexpected exceptions with traceback.Improves diagnostics without changing behavior.
- except Exception as exc: # pylint: disable=broad-exception-caught - logger.error("Unexpected exception: %s", exc) + except Exception: # pylint: disable=broad-exception-caught + logger.exception("Unexpected exception") return _error_response(500, "internal", "Unexpected server error")
44-44: Security: stop disabling TLS verification and warning suppression.Enable certificate verification by default and keep warnings on; add timeouts only.
-urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) + # Keep TLS warnings enabled; if a custom CA is needed, set REQUESTS_CA_BUNDLE/AWS_CA_BUNDLE env vars. @@ -aws_s3 = boto3.Session().resource("s3", verify=False) # nosec Boto verify disabled intentionally +aws_s3 = boto3.Session().resource("s3") @@ -response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external +response_json = requests.get(CONFIG["token_public_key_url"], timeout=5).json()To confirm no other insecure disables remain, run:
#!/bin/bash rg -nP 'verify\s*=\s*False|disable_warnings\(' -g '!**/venv/**' -g '!**/.venv/**' -C2Also applies to: 73-73, 88-88
🧹 Nitpick comments (14)
src/writer_kafka.py (2)
75-83: Minor: tighten types and logging; silence unused lambda param.Use list[str], parameterized logging, and underscore for unused arg.
- errors: list[Any] = [] + errors: list[str] = [] @@ - logger.debug(f"Sending to kafka {topic_name}") + logger.debug("Sending to Kafka %s", topic_name) @@ - callback=lambda err, msg: (errors.append(str(err)) if err is not None else None), + callback=lambda err, _msg: errors.append(str(err)) if err else None,
85-88: Log with traceback and return the producer error string.Avoid “unknown error” phrasing; keep traceback in logs and return the actual error text.
- except KafkaException as e: # narrow exception capture - err_msg = f"The Kafka writer failed with unknown error: {str(e)}" - logger.exception(err_msg) - return False, err_msg + except KafkaException as e: # narrow exception capture + logger.exception("Kafka producer error") + return False, str(e)src/event_gate_lambda.py (1)
256-264: Defensive access to pathParameters/body to avoid KeyError/TypeError.Keeps handler resilient to malformed events.
- if method == "GET": - return get_topic_schema(event["pathParameters"]["topic_name"].lower()) + if method == "GET": + topic = (event.get("pathParameters") or {}).get("topic_name", "") + return get_topic_schema(topic.lower()) @@ - if method == "POST": - return post_topic_message( - event["pathParameters"]["topic_name"].lower(), - json.loads(event["body"]), - extract_token(event.get("headers", {})), - ) + if method == "POST": + topic = (event.get("pathParameters") or {}).get("topic_name", "") + body = json.loads(event.get("body") or "{}") + return post_topic_message(topic.lower(), body, extract_token(event.get("headers", {})))tests/test_event_gate_lambda.py (2)
42-53: Nit: mark unused args, drop unused noqa.Silences ARG/RUF nits without behavior changes.
- class DummyProducer: # minimal interface - def __init__(self, *a, **kw): + class DummyProducer: # minimal interface + def __init__(self, *_args, **_kwargs): pass @@ - def produce(self, *a, **kw): - cb = kw.get("callback") + def produce(self, *_args, **kwargs): + cb = kwargs.get("callback") if cb: cb(None, None) @@ - def flush(self): # noqa: D401 - simple stub + def flush(self): return None
89-95: Nit: remove unused noqa and mark unused params.Minor cleanup for test stubs.
- def Object(self, key): # noqa: D401 - simple proxy - return MockS3Object() + def Object(self, _key): + return MockS3Object() @@ - def Bucket(self, name): # noqa: D401 - simple proxy - return MockS3Bucket() + def Bucket(self, _name): + return MockS3Bucket()tests/test_conf_path.py (9)
23-29: Avoid repo-name coupling; assert using a computed parent path.Endswith("EventGate/conf") is brittle and OS-separator sensitive. Compute the expected parent conf path from the module location.
def test_env_var_invalid_directory_falls_back_parent(monkeypatch): missing_path = "/nonexistent/path/xyz_does_not_exist" monkeypatch.setenv("CONF_DIR", missing_path) conf_dir, invalid = conf_path_module.resolve_conf_dir() - # Should fall back to repository conf directory - assert conf_dir.endswith(os.path.join("EventGate", "conf")) + # Should fall back to repository conf directory (computed relative to module) + expected_parent_conf = (Path(conf_path_module.__file__).resolve().parent.parent / "conf").resolve() + assert Path(conf_dir).resolve() == expected_parent_conf assert invalid == os.path.abspath(missing_path)
7-7: Remove unused import.
pytestisn’t referenced directly.-import pytest
37-38: Drop unused import.
inspectis never used; it will trigger pylint warnings.- import inspect
45-51: Guard against None spec/loader; drop type: ignore.This tightens types (mypy) and avoids potential
Nonederef.- spec = importlib.util.spec_from_file_location(f"conf_path_isolated_{id(module_dir)}", module_file) - mod = importlib.util.module_from_spec(spec) - sys.modules[spec.name] = mod # ensure import works for dependencies - spec.loader.exec_module(mod) # type: ignore[attr-defined] + spec = importlib.util.spec_from_file_location(f"conf_path_isolated_{id(module_dir)}", module_file) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = mod # ensure import works for dependencies + spec.loader.exec_module(mod)
54-71: Make test immune to ambient CONF_DIR and compare full paths.Ensure no external env interferes; avoid endswith path checks.
-def test_current_dir_conf_used_when_parent_missing(): +def test_current_dir_conf_used_when_parent_missing(monkeypatch): @@ - mod = _load_isolated_conf_path(build) + mod = _load_isolated_conf_path(build) try: + # Ensure no env overrides + monkeypatch.delenv("CONF_DIR", raising=False) conf_dir, invalid = mod.resolve_conf_dir() - assert conf_dir.endswith("pkg/conf") # current directory conf chosen + expected_current = (Path(mod.__file__).parent / "conf").resolve() + assert Path(conf_dir).resolve() == expected_current # current directory conf chosen assert invalid is None
73-90: Also unset CONF_DIR here to avoid flakiness.Keeps behavior deterministic regardless of runner env.
-def test_fallback_parent_conf_even_if_missing(): +def test_fallback_parent_conf_even_if_missing(monkeypatch): @@ - mod = _load_isolated_conf_path(build) + mod = _load_isolated_conf_path(build) try: + monkeypatch.delenv("CONF_DIR", raising=False) conf_dir, invalid = mod.resolve_conf_dir()
102-111: Prefer path equality over suffix match.More portable across OSes and layout differences.
conf_dir, invalid = mod.resolve_conf_dir() - assert conf_dir.endswith("pkg_invalid_current/conf") + expected_current = (Path(mod.__file__).parent / "conf").resolve() + assert Path(conf_dir).resolve() == expected_current assert invalid == os.path.abspath(bad_path)
134-147: Tighten importlib usage and clean up sys.modules.Add spec/loader asserts, remove type ignore, and pop the temporary module.
spec = importlib.util.spec_from_file_location("conf_path_valid_mod", conf_path_module.__file__) + assert spec is not None and spec.loader is not None mod = importlib.util.module_from_spec(spec) sys.modules[spec.name] = mod - spec.loader.exec_module(mod) # type: ignore[attr-defined] + spec.loader.exec_module(mod) assert mod.CONF_DIR == str(valid_conf) # type: ignore[attr-defined] assert mod.INVALID_CONF_ENV is None # type: ignore[attr-defined] + sys.modules.pop(spec.name, None)
149-160: Avoid repo-name assumption; assert against computed parent path. Also assert spec/loader and clean sys.modules.Improves portability and typing hygiene.
bad_path = "/no/such/dir/abcXYZ123" monkeypatch.setenv("CONF_DIR", bad_path) spec = importlib.util.spec_from_file_location("conf_path_invalid_mod", conf_path_module.__file__) + assert spec is not None and spec.loader is not None mod = importlib.util.module_from_spec(spec) sysmodules_before = dict(sys.modules) sys.modules[spec.name] = mod - spec.loader.exec_module(mod) # type: ignore[attr-defined] + spec.loader.exec_module(mod) # Module constant should fall back to repository conf directory - assert mod.CONF_DIR.endswith(os.path.join("EventGate", "conf")) # type: ignore[attr-defined] + expected_parent_conf = (Path(mod.__file__).parent.parent / "conf").resolve() + assert Path(mod.CONF_DIR).resolve() == expected_parent_conf # type: ignore[attr-defined] assert mod.INVALID_CONF_ENV == os.path.abspath(bad_path) # type: ignore[attr-defined] + sys.modules.pop(spec.name, None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/event_gate_lambda.py(5 hunks)src/writer_kafka.py(1 hunks)tests/test_conf_path.py(1 hunks)tests/test_event_gate_lambda.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_conf_path.py (1)
src/conf_path.py (1)
resolve_conf_dir(26-54)
tests/test_event_gate_lambda.py (2)
src/writer_kafka.py (1)
KafkaException(16-17)src/event_gate_lambda.py (1)
lambda_handler(242-270)
src/writer_kafka.py (2)
src/writer_eventbridge.py (2)
init(16-26)write(35-79)tests/test_event_gate_lambda.py (3)
get(85-86)produce(46-49)flush(51-52)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (4)
get(85-86)read(75-82)Bucket(93-94)Object(89-90)src/writer_kafka.py (2)
init(23-56)write(59-95)src/writer_eventbridge.py (2)
init(16-26)write(35-79)src/writer_postgres.py (2)
init(23-44)write(226-268)
🪛 Ruff (0.12.2)
tests/test_event_gate_lambda.py
46-46: Unused method argument: a
(ARG002)
51-51: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
89-89: Unused method argument: key
(ARG002)
89-89: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
93-93: Unused method argument: name
(ARG002)
93-93: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
src/writer_kafka.py
35-35: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Unused lambda argument: msg
(ARG005)
86-86: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/event_gate_lambda.py
88-88: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
242-242: Unused function argument: context
(ARG001)
268-268: Do not catch blind exception: Exception
(BLE001)
269-269: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
tests/test_conf_path.py (1)
12-20: LGTM: env var happy-path test is correct.Covers the valid CONF_DIR case accurately.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_extract_token.py (1)
71-73: Assert the extracted token to avoid a false-positive test.Currently the variable is unused.
def test_extract_token_direct_bearer_header(egl_mod): - token = egl_mod.extract_token({"Bearer": " tok123 "}) + token = egl_mod.extract_token({"Bearer": " tok123 "}) + assert token == "tok123"
🧹 Nitpick comments (10)
tests/test_writer_kafka.py (4)
8-12: Silence Ruff ARG/RUF100 by renaming unused args and dropping unused noqa.Keep the interface but avoid warnings.
class FakeProducerSuccess: - def __init__(self, *a, **kw): + def __init__(self, *_a, **_kw): self.produced = [] - def produce(self, topic, key, value, callback): # noqa: D401 + def produce(self, topic, key, value, callback): self.produced.append((topic, key, value)) # simulate success callback(None, SimpleNamespace()) class FakeProducerError(FakeProducerSuccess): - def produce(self, topic, key, value, callback): # noqa: D401 + def produce(self, _topic, _key, _value, callback): # simulate async error callback("ERR", None) class RaisingProducer(FakeProducerSuccess): - def produce(self, *a, **kw): # noqa: D401 + def produce(self, *_a, **_kw): raise DummyKafkaException("boom")Also applies to: 20-23, 55-56
26-30: Remove unused pytest fixture parametermonkeypatch.It’s not used; dropping it silences ARG001.
-def test_write_skips_when_producer_none(monkeypatch): +def test_write_skips_when_producer_none(): @@ -def test_write_success(monkeypatch): +def test_write_success(): @@ -def test_write_async_error(monkeypatch): +def test_write_async_error():Also applies to: 33-38, 40-45
32-37: Optional: assert that the payload was actually produced.Strengthens the success-path assertion.
wk.STATE["producer"] = FakeProducerSuccess() ok, err = wk.write("topic", {"b": 2}) assert ok and err is None +assert wk.STATE["producer"].produced == [("topic", None, json.dumps({"b": 2}).encode("utf-8"))]
26-63: Optional: add a STATE snapshot fixture to prevent cross-test leakage.Keeps module-level STATE isolated across tests.
# add near imports in this file import copy import pytest @pytest.fixture(autouse=True) def _snapshot_state(): saved = copy.deepcopy(wk.STATE) try: yield finally: wk.STATE.clear() wk.STATE.update(saved)tests/test_event_gate_lambda_local_access.py (1)
1-4: Fix lint and improve the S3-branch guard.
- Use builtins.open, remove unused noqa.
- Replace AssertionError with pytest.fail for clearer test intent.
- Silence unused arg in Bucket.
import importlib import io import json from unittest.mock import patch, MagicMock +import builtins +import pytest @@ -original_open = open # noqa: A001 +original_open = builtins.open @@ -def open_side_effect(path, *args, **kwargs): # noqa: D401 +def open_side_effect(path, *args, **kwargs): @@ class MockS3: - def Bucket(self, name): # noqa: D401 - raise AssertionError("S3 branch should not be used for local access_config") + def Bucket(self, _name): + pytest.fail("S3 path used unexpectedly during local access_config")Also applies to: 10-11, 26-33, 45-50
tests/test_writer_eventbridge.py (2)
2-2: Drop unused import.-from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock
44-51: Make DummyError instantiation robust against botocore’s init signature.Some versions require an error_message. This avoids TypeError at setup.
-class DummyError(BotoCoreError): - pass +class DummyError(BotoCoreError): + def __init__(self): + super().__init__(error_message="boom") @@ - mock_client.put_events.side_effect = DummyError() + mock_client.put_events.side_effect = DummyError()tests/test_extract_token.py (1)
41-47: Silence Ruff by removing unused noqa and args.class MockBucket: - def Object(self, key): # noqa: D401 + def Object(self, _key): return MockObject() class MockS3: - def Bucket(self, name): # noqa: D401 + def Bucket(self, _name): return MockBucket()tests/test_event_gate_lambda.py (2)
43-53: Tidy unused args and remove unused noqa markers.Minor polish to satisfy Ruff without changing behavior.
class DummyProducer: # minimal interface - def __init__(self, *a, **kw): + def __init__(self, *_a, **_kw): pass - def flush(self): # noqa: D401 - simple stub + def flush(self): return None @@ class MockS3Bucket: - def Object(self, key): # noqa: D401 - simple proxy + def Object(self, _key): return MockS3Object() @@ class MockS3Resource: - def Bucket(self, name): # noqa: D401 - simple proxy + def Bucket(self, _name): return MockS3Bucket()Also applies to: 89-95
321-333: Consider returning 400 for invalid JSON (optional).Currently invalid JSON yields 500 internal; treating it as a bad request may be preferable.
If you choose to change the handler to return 400 on JSON decode error, I can provide a targeted patch and update the test accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/test_event_gate_lambda.py(1 hunks)tests/test_event_gate_lambda_local_access.py(1 hunks)tests/test_extract_token.py(1 hunks)tests/test_writer_eventbridge.py(1 hunks)tests/test_writer_kafka.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_extract_token.py (1)
src/event_gate_lambda.py (1)
extract_token(207-239)
tests/test_writer_kafka.py (1)
tests/test_event_gate_lambda.py (3)
produce(46-49)flush(51-52)DummyKafkaException(56-57)
tests/test_event_gate_lambda_local_access.py (2)
tests/test_extract_token.py (2)
MockS3(44-46)Bucket(45-46)tests/test_event_gate_lambda.py (1)
Bucket(93-94)
tests/test_writer_eventbridge.py (1)
tests/test_writer_kafka.py (1)
test_write_success(33-37)
tests/test_event_gate_lambda.py (2)
src/writer_kafka.py (1)
KafkaException(16-17)src/event_gate_lambda.py (1)
lambda_handler(242-270)
🪛 Ruff (0.12.2)
tests/test_extract_token.py
41-41: Unused method argument: key
(ARG002)
41-41: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
45-45: Unused method argument: name
(ARG002)
45-45: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
72-72: Local variable token is assigned to but never used
Remove assignment to unused variable token
(F841)
tests/test_writer_kafka.py
8-8: Unused method argument: a
(ARG002)
8-8: Unused method argument: kw
(ARG002)
11-11: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
21-21: Unused method argument: topic
(ARG002)
21-21: Unused method argument: key
(ARG002)
21-21: Unused method argument: value
(ARG002)
21-21: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
26-26: Unused function argument: monkeypatch
(ARG001)
33-33: Unused function argument: monkeypatch
(ARG001)
40-40: Unused function argument: monkeypatch
(ARG001)
55-55: Unused method argument: a
(ARG002)
55-55: Unused method argument: kw
(ARG002)
55-55: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
tests/test_event_gate_lambda_local_access.py
10-10: Unused noqa directive (non-enabled: A001)
Remove unused noqa directive
(RUF100)
26-26: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
46-46: Unused method argument: name
(ARG002)
46-46: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_event_gate_lambda.py
46-46: Unused method argument: a
(ARG002)
51-51: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
89-89: Unused method argument: key
(ARG002)
89-89: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
93-93: Unused method argument: name
(ARG002)
93-93: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (5)
tests/test_event_gate_lambda_local_access.py (1)
57-58: Good coverage of the local-config branch.Reload + patched open ensures the local path is exercised; assertions look right.
tests/test_writer_eventbridge.py (1)
25-39: Nice negative-case coverage.The failed entry path is asserted clearly; message coupling (“EventBridge”) is acceptable here.
tests/test_extract_token.py (1)
8-65: Fixture is solid.Patch surface is tight, teardown is explicit; good isolation.
tests/test_event_gate_lambda.py (2)
63-66: Good: non-clobbering psycopg2 stub.The find_spec + patch.dict approach avoids masking a real install.
209-228: Solid aggregation tests across writers.Happy-path and single/multi-failure cases are well exercised with precise assertions.
Also applies to: 230-250, 252-270
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/writer_kafka.py (1)
42-55: Validate all required SASL/SSL keys before indexing config.Direct indexing will raise KeyError when any key is missing; validate and fail fast with a clear message. This was flagged previously.
- if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: - producer_config.update( + if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if not config.get(k)] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": config["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": config["kafka_sasl_kerberos_principal"], "ssl.ca.location": config["kafka_ssl_ca_path"], "ssl.certificate.location": config["kafka_ssl_cert_path"], "ssl.key.location": config["kafka_ssl_key_path"], "ssl.key.password": config["kafka_ssl_key_password"], } )
🧹 Nitpick comments (7)
src/writer_kafka.py (4)
85-85: Silence ARG005: name the unused callback argument.Rename to underscore to satisfy Ruff.
- callback=lambda err, msg: (errors.append(str(err)) if err is not None else None), + callback=lambda err, _msg: errors.append(str(err)) if err else None,
80-80: Use parameterized logging and log exceptions with traceback; return the exception string.Avoid f-strings in logs (RUF010) and keep the traceback via logger.exception.
- logger.debug(f"Sending to kafka {topic_name}") + logger.debug("Sending to Kafka %s", topic_name) @@ - except KafkaException as e: # narrow exception capture - err_msg = f"The Kafka writer failed with unknown error: {str(e)}" - logger.exception(err_msg) - return False, err_msg + except KafkaException as err: # narrow exception capture + logger.exception("Kafka producer error") + return False, str(err) @@ - logger.error(msg) + logger.error("%s", msg)Also applies to: 96-104
78-78: Tighten type: errors holds strings, not Any.- errors: list[Any] = [] + errors: list[str] = []
11-11: Mypy: ignore untyped confluent_kafka import (if not globally ignored).If mypy doesn’t have stubs for confluent_kafka and ignore_missing_imports is not set, add an import ignore.
-from confluent_kafka import Producer +from confluent_kafka import Producer # type: ignore[import-untyped]If you prefer config, ensure mypy sets ignore_missing_imports = true for this package.
tests/test_event_gate_lambda.py (3)
51-51: Remove unused noqa D401 directives.These codes aren’t enabled; Ruff flags them as unused (RUF100).
- def flush(self): # noqa: D401 - simple stub + def flush(self): @@ - def Object(self, key): # noqa: D401 - simple proxy + def Object(self, key): @@ - def Bucket(self, name): # noqa: D401 - simple proxy + def Bucket(self, name):Also applies to: 89-89, 93-93
207-209: Drop unused DummyJwtError.It’s defined but not used.
- class DummyJwtError(Exception): - pass -
29-36: Make fixture teardown exception-safe with ExitStack context.If an exception occurs before yield, started patches won’t be stopped. Wrap the body in
with ExitStack() as exit_stack:and remove manual cleanup.Example sketch:
- started_patches = [] - exit_stack = ExitStack() + started_patches = [] + with ExitStack() as exit_stack: @@ - for p in started_patches: - p.stop() - exit_stack.close() + # remove manual cleanup; ExitStack handles it even on exceptionsAlso applies to: 112-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/writer_kafka.py(1 hunks)tests/test_event_gate_lambda.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_event_gate_lambda.py (4)
tests/test_extract_token.py (5)
start(12-15)get(37-38)read(27-34)Object(41-42)Bucket(45-46)tests/test_writer_kafka.py (5)
produce(11-14)produce(21-23)produce(55-56)flush(16-17)DummyKafkaException(47-48)src/writer_kafka.py (1)
KafkaException(17-18)src/event_gate_lambda.py (1)
lambda_handler(242-270)
src/writer_kafka.py (2)
tests/test_event_gate_lambda.py (3)
get(85-86)produce(46-49)flush(51-52)src/writer_eventbridge.py (2)
init(16-26)write(35-79)
🪛 Ruff (0.12.2)
tests/test_event_gate_lambda.py
46-46: Unused method argument: a
(ARG002)
51-51: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
89-89: Unused method argument: key
(ARG002)
89-89: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
93-93: Unused method argument: name
(ARG002)
93-93: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
src/writer_kafka.py
38-38: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Unused lambda argument: msg
(ARG005)
97-97: Use explicit conversion flag
Replace with conversion flag
(RUF010)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
DEVELOPER.md (3)
71-74: Add language to fenced block (Expected Output).Specify a language to satisfy MD040 and improve readability.
-``` +```text All done! ✨ 🍰 ✨ 1 file reformatted.--- `94-99`: **Align mypy example with the stated pattern.** Either drop the flag from the sentence or include it in the example; keeping it in the example is clearer. ```diff -```shell -mypy src/writer_kafka.py -``` +```shell +mypy src/writer_kafka.py --check-untyped-defs +```
135-138: Make “open coverage report” cross‑platform.Provide Linux/Windows variants in addition to macOS.
-```shell -open htmlcov/index.html -``` +```bash +# macOS +open htmlcov/index.html +# Linux +xdg-open htmlcov/index.html +# Windows (PowerShell) +start htmlcov\index.html +```README.md (5)
127-131: Add languages to fenced code blocks (bash).Addresses MD040 across multiple blocks.
-``` +```bash python -m venv .venv source .venv/bin/activate pip install -r requirements.txt-
+bash
pytest -q-``` +```bash pytest --cov=. --cov-report=term-missing --cov-fail-under=80-
+bash
pytest tests/test_writer_kafka.py -q
pytest -k kafka -q-``` +```bash mypy . pylint $(git ls-files '*.py')-
+bash
black $(git ls-files '*.py')Also applies to: 133-135, 137-139, 141-144, 146-149, 151-153
162-167: Add language to Python snippet.Improves highlighting and satisfies MD040.
-``` +```python from src import event_gate_lambda as m resp = m.lambda_handler({"resource": "/topics"}, None) print(resp)--- `194-203`: **Surround the table with blank lines.** Fixes MD058 and renders consistently across viewers. ```diff ## Troubleshooting + | Symptom | Possible Cause | Action | |---------|----------------|--------| | 401 Unauthorized | Missing / malformed token header | Ensure `Authorization: Bearer` present | | 403 Forbidden | Subject not listed in access map | Update `access.json` and redeploy / reload | | 404 Topic not found | Wrong casing or not loaded in code | Verify loaded topics & file names | | 500 Writer failure | Downstream (Kafka / EventBridge / DB) unreachable | Check network / VPC endpoints / security groups | | Lambda keeps old config | Warm container | Call `/terminate` (internal) to force cold start | + ## License
52-53: Prefix writers path with src/ for consistency.Other “Key files” entries use explicit roots.
-- `writer_*.py` – individual sink implementations +- `src/writer_*.py` – individual sink implementations
101-101: Rename script to correct spelling and update README
Script filescripts/prepare.deplyoment.shis misspelled; rename it toscripts/prepare.deployment.sh, keep the old name as a deprecated alias for backward-compatibility, and update README.md at lines 101 and 190–191 accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
DEVELOPER.md(1 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
DEVELOPER.md
9-9: Link fragments should be valid
(MD051, link-fragments)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
195-195: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
DEVELOPER.md (1)
15-17: Repo directory case fixed — thanks.The cd path now matches the cloned directory name.
🧹 Nitpick comments (5)
DEVELOPER.md (5)
68-73: Fix MD040: add a language to the “Expected Output” code fence.Add a language hint (text) to satisfy markdownlint and improve rendering.
-``` +```text All done! ✨ 🍰 ✨ 1 file reformatted.--- `134-139`: **Make the coverage report open command cross‑platform.** `open` is macOS-only. Add a portable fallback so Linux/Windows devs aren’t blocked. ```diff Open the HTML coverage report: ```shell -open htmlcov/index.html +# macOS +open htmlcov/index.html +# Linux +xdg-open htmlcov/index.html +# Any OS (portable) +python -m webbrowser -t htmlcov/index.html--- `20-24`: **Polish env setup: use python -m pip and show Windows venv activation.** Improves reliability and helps Windows users. ```diff ```shell python3 -m venv venv -source venv/bin/activate -pip install -r requirements.txt +source venv/bin/activate # on macOS/Linux +# .\venv\Scripts\activate # on Windows PowerShell +python -m pip install --upgrade pip +python -m pip install -r requirements.txt--- `93-99`: **Align example with the flag mentioned above.** Text says to use `--check-untyped-defs`, but the example omits it. ```diff Example: ```shell -mypy src/writer_kafka.py +mypy src/writer_kafka.py --check-untyped-defs--- `8-8`: **Consistency nit: pluralize “Unit Test(s)”.** Use “Unit Tests” in both TOC and heading for consistency. ```diff -- [Run Unit Test](#running-unit-test) +- [Run Unit Tests](#running-unit-tests) @@ -## Running Unit Test +## Running Unit TestsAlso applies to: 100-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
DEVELOPER.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
DEVELOPER.md
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/writer_kafka.py (1)
58-70: Validate SASL/SSL keys before indexing the configAccessing several keys without checking can raise KeyError at runtime. Validate the full set before update.
- if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: - producer_config.update( + if "kafka_sasl_kerberos_principal" in config and "kafka_ssl_key_path" in config: + required = { + "kafka_sasl_kerberos_keytab_path", + "kafka_sasl_kerberos_principal", + "kafka_ssl_ca_path", + "kafka_ssl_cert_path", + "kafka_ssl_key_path", + "kafka_ssl_key_password", + } + missing = [k for k in required if k not in config] + if missing: + raise KeyError(f"Kafka SASL/SSL config missing keys: {missing}") + producer_config.update( { "security.protocol": "SASL_SSL", "sasl.mechanism": "GSSAPI", "sasl.kerberos.service.name": "kafka", "sasl.kerberos.keytab": config["kafka_sasl_kerberos_keytab_path"], "sasl.kerberos.principal": config["kafka_sasl_kerberos_principal"], "ssl.ca.location": config["kafka_ssl_ca_path"], "ssl.certificate.location": config["kafka_ssl_cert_path"], "ssl.key.location": config["kafka_ssl_key_path"], "ssl.key.password": config["kafka_ssl_key_password"], } )DEVELOPER.md (1)
21-24: Prefer .venv to avoid polluting the project root; align with prior feedback.Using a hidden virtualenv is a common convention and matches the earlier review suggestion.
-python3 -m venv venv -source venv/bin/activate +python3 -m venv .venv +source .venv/bin/activate pip install -r requirements.txtsrc/writer_postgres.py (3)
29-33: Remove unused noqa directive on psycopg2 import.Ruff flags this as RUF100; the symbol is used later so the noqa is unnecessary.
- import psycopg2 # noqa: F401 + import psycopg2
294-297: Fix grammar and log with traceback on DB errors.Use logger.exception and correct the message wording; keeps tests compatible if they assert substrings.
- except (RuntimeError, PsycopgError) as e: # narrowed exception set - err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + except (RuntimeError, PsycopgError) as e: # narrowed exception set + err_msg = f"The Postgres writer failed with unknown error: {e}" + _logger.exception(err_msg) + return False, err_msg
68-74: Harden secret retrieval: add error handling and keep service resilient.Failures in AWS Secrets Manager or JSON parsing will currently raise at init; catch and fall back.
- if secret_name and secret_region: - aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) - postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] - POSTGRES = json.loads(postgres_secret) + if secret_name and secret_region: + try: + aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) + resp = aws_secrets.get_secret_value(SecretId=secret_name) + postgres_secret = resp.get("SecretString", "{}") + POSTGRES = json.loads(postgres_secret) + except Exception: # narrow as desired (e.g., requests/botocore/JSON errors) + _logger.exception("Failed to fetch Postgres secret") + POSTGRES = {"database": ""}src/event_gate_lambda.py (2)
25-26: Re-enable TLS verification; remove verify=False and warning suppression.Disabling TLS is a security risk (S501). Let clients use system CAs or env-provided bundles; add timeouts.
-import urllib3 @@ -urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) @@ -aws_s3 = boto3.Session().resource("s3", verify=False) # nosec Boto verify disabled intentionally +aws_s3 = boto3.Session().resource("s3") @@ -response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external +response_json = requests.get(CONFIG["token_public_key_url"], timeout=10).json()Optionally support a custom CA with REQUESTS_CA_BUNDLE by passing verify=os.environ.get("REQUESTS_CA_BUNDLE") to requests.get.
Also applies to: 46-46, 75-77, 90-93
270-272: Log unexpected exceptions with traceback.Use logger.exception for diagnostics.
- except Exception as exc: # pylint: disable=broad-exception-caught - logger.error("Unexpected exception: %s", exc) + except Exception: # pylint: disable=broad-exception-caught + logger.exception("Unexpected exception") return _error_response(500, "internal", "Unexpected server error")
🧹 Nitpick comments (14)
src/writer_eventbridge.py (2)
64-69: Config policy inconsistency: skip vs. fail-fastKafka raises on missing required config, but EventBridge silently skips when ARN/client is missing. Please confirm this is intentional and consistent with system expectations; otherwise consider failing early in init.
65-65: Nit: consistent naming/capitalization in logsUse “EventBridge” and “ARN” consistently in log messages.
- logger.debug("No EventBus Arn - skipping") + logger.debug("No EventBridge ARN - skipping") @@ - logger.debug("Sending to eventBridge %s", topic_name) + logger.debug("Sending to EventBridge %s", topic_name)Also applies to: 72-72
src/conf_path.py (2)
25-25: Add return type hints for mypyAnnotate the function and import typing to satisfy static checks.
+from typing import Optional, Tuple @@ -def resolve_conf_dir(env_var: str = "CONF_DIR"): +def resolve_conf_dir(env_var: str = "CONF_DIR") -> Tuple[str, Optional[str]]:Also applies to: 28-36
57-59: Nit: normalize path to absolute for consistencyMake current_conf absolute like other paths.
- current_conf = os.path.join(current_dir, "conf") + current_conf = os.path.abspath(os.path.join(current_dir, "conf"))src/writer_kafka.py (3)
96-97: Improve logging (capitalization and exception formatting)Tidy log text and avoid preformatted exception strings; keeps traceback and satisfies Ruff RUF010/TRY003.
- logger.debug("Sending to kafka %s", topic_name) + logger.debug("Sending to Kafka %s", topic_name) @@ - except KafkaException as e: # narrow exception capture - err_msg = f"The Kafka writer failed with unknown error: {str(e)}" - logger.exception(err_msg) - return False, err_msg + except KafkaException as e: # narrow exception capture + logger.exception("Kafka producer error") + return False, str(e)Also applies to: 112-115
101-101: Fix unused lambda argument in callbackRename the unused parameter to “_” to satisfy ARG005.
- callback=lambda err, msg: (errors.append(str(err)) if err is not None else None), + callback=lambda err, _: (errors.append(str(err)) if err is not None else None),
27-27: Optional: silence mypy for untyped confluent_kafka importIf mypy flags this import as untyped, add an ignore.
-from confluent_kafka import Producer +from confluent_kafka import Producer # type: ignore[import-untyped]DEVELOPER.md (2)
68-73: Fix MD040: add language to fenced code block.Add a language hint to the “Expected Output” code fence to satisfy markdownlint.
-``` +```text All done! ✨ 🍰 ✨ 1 file reformatted. -``` +```
134-137: Make coverage report open command cross‑platform.Use Python’s webbrowser to work on Linux/macOS/Windows.
-open htmlcov/index.html +python -m webbrowser htmlcov/index.htmlsrc/writer_postgres.py (4)
86-134: Avoid f-strings for identifiers; use psycopg2.sql to safely inject table names.Prevents accidental SQL injection if a table ever becomes dynamic and improves correctness with quoting.
- cursor.execute( - f""" - INSERT INTO {table} + from psycopg2 import sql # safe: only executed when psycopg2 is present + cursor.execute( + sql.SQL(""" + INSERT INTO {} ( event_id, @@ - )""", + )""").format(sql.Identifier(table)), ( message["event_id"], @@ - (json.dumps(message.get("format_options")) if "format_options" in message else None), - (json.dumps(message.get("additional_info")) if "additional_info" in message else None), + (json.dumps(message.get("format_options")) if "format_options" in message else None), + (json.dumps(message.get("additional_info")) if "additional_info" in message else None), ), )Apply the same pattern to the INSERTs in Lines 146-180 and 183-214, and Lines 226-254.
146-180: Apply safe identifier formatting in runs/jobs inserts as well.Mirror the sql.Identifier approach for both {table_runs} and {table_jobs}.
Also applies to: 183-214
226-254: Apply safe identifier formatting in test insert.
293-293: Drop redundant commit inside connection context manager.psycopg2 commits on successful context exit; double commit is unnecessary.
- connection.commit() # type: ignore + # Commit handled by context manager on successful exitsrc/event_gate_lambda.py (1)
258-266: Return 400 for invalid JSON bodies instead of 500.Improve UX by distinguishing client errors.
- if method == "POST": - return post_topic_message( - event["pathParameters"]["topic_name"].lower(), - json.loads(event["body"]), - extract_token(event.get("headers", {})), - ) + if method == "POST": + try: + body = json.loads(event["body"]) + except Exception: + return _error_response(400, "validation", "Request body must be valid JSON") + return post_topic_message( + event["pathParameters"]["topic_name"].lower(), + body, + extract_token(event.get("headers", {})), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
DEVELOPER.md(1 hunks)src/conf_path.py(1 hunks)src/event_gate_lambda.py(5 hunks)src/writer_eventbridge.py(1 hunks)src/writer_kafka.py(1 hunks)src/writer_postgres.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/writer_kafka.py (3)
tests/test_event_gate_lambda.py (3)
get(85-86)produce(46-49)flush(51-52)src/writer_eventbridge.py (2)
init(32-42)write(51-95)src/writer_postgres.py (2)
init(54-75)write(257-299)
src/writer_eventbridge.py (2)
src/writer_kafka.py (2)
init(42-75)write(78-122)src/writer_postgres.py (2)
init(54-75)write(257-299)
src/writer_postgres.py (3)
src/writer_eventbridge.py (2)
init(32-42)write(51-95)src/writer_kafka.py (2)
init(42-75)write(78-122)tests/test_writer_postgres.py (5)
client(244-245)cursor(175-176)connect(192-193)connect(228-229)commit(178-179)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (4)
get(85-86)read(75-82)Bucket(93-94)Object(89-90)src/writer_eventbridge.py (2)
init(32-42)write(51-95)src/writer_kafka.py (2)
init(42-75)write(78-122)src/writer_postgres.py (2)
init(54-75)write(257-299)
🪛 Ruff (0.12.2)
src/writer_kafka.py
54-54: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Unused lambda argument: msg
(ARG005)
113-113: Use explicit conversion flag
Replace with conversion flag
(RUF010)
src/writer_postgres.py
30-30: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
src/event_gate_lambda.py
90-90: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
244-244: Unused function argument: context
(ARG001)
270-270: Do not catch blind exception: Exception
(BLE001)
271-271: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 markdownlint-cli2 (0.17.2)
DEVELOPER.md
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
src/event_gate_lambda.py (2)
209-241: Token extraction is robust and clear.Case-insensitive handling and legacy header support look good.
2-2: License year range—confirm project convention.Prior feedback suggests preserving the project’s start year. If applicable, use the original start year (e.g., 2023–2025).
| TOKEN_PROVIDER_URL = CONFIG["token_provider_url"] | ||
| token_public_key_encoded = requests.get( | ||
| CONFIG["token_public_key_url"], verify=False | ||
| ).json()["key"] | ||
| TOKEN_PUBLIC_KEY = serialization.load_der_public_key( | ||
| base64.b64decode(token_public_key_encoded) | ||
| ) | ||
| # Add timeout to avoid hanging requests | ||
| response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external | ||
| token_public_key_encoded = response_json["key"] | ||
| TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(base64.b64decode(token_public_key_encoded)) | ||
| logger.debug("Loaded TOKEN_PUBLIC_KEY") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle token public key fetch failures explicitly.
Avoid crashing on bad responses; log with traceback.
-TOKEN_PROVIDER_URL = CONFIG["token_provider_url"]
-# Add timeout to avoid hanging requests
-response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external
-token_public_key_encoded = response_json["key"]
-TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(base64.b64decode(token_public_key_encoded))
+TOKEN_PROVIDER_URL = CONFIG["token_provider_url"]
+try:
+ resp = requests.get(CONFIG["token_public_key_url"], timeout=10)
+ resp.raise_for_status()
+ token_public_key_encoded = resp.json()["key"]
+ TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(base64.b64decode(token_public_key_encoded))
+except Exception:
+ logger.exception("Failed to load TOKEN_PUBLIC_KEY")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TOKEN_PROVIDER_URL = CONFIG["token_provider_url"] | |
| token_public_key_encoded = requests.get( | |
| CONFIG["token_public_key_url"], verify=False | |
| ).json()["key"] | |
| TOKEN_PUBLIC_KEY = serialization.load_der_public_key( | |
| base64.b64decode(token_public_key_encoded) | |
| ) | |
| # Add timeout to avoid hanging requests | |
| response_json = requests.get(CONFIG["token_public_key_url"], verify=False, timeout=5).json() # nosec external | |
| token_public_key_encoded = response_json["key"] | |
| TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key(base64.b64decode(token_public_key_encoded)) | |
| logger.debug("Loaded TOKEN_PUBLIC_KEY") | |
| TOKEN_PROVIDER_URL = CONFIG["token_provider_url"] | |
| try: | |
| resp = requests.get(CONFIG["token_public_key_url"], timeout=10) | |
| resp.raise_for_status() | |
| token_public_key_encoded = resp.json()["key"] | |
| TOKEN_PUBLIC_KEY: Any = serialization.load_der_public_key( | |
| base64.b64decode(token_public_key_encoded) | |
| ) | |
| except Exception: | |
| logger.exception("Failed to load TOKEN_PUBLIC_KEY") | |
| raise |
🧰 Tools
🪛 Ruff (0.12.2)
90-90: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
🤖 Prompt for AI Agents
In src/event_gate_lambda.py around lines 88 to 93, the token public key fetch
and deserialization are done without error handling which causes unhandled
crashes on bad responses; wrap the entire requests.get(...).json() and
serialization.load_der_public_key(...) sequence in a try/except that catches
requests.RequestException, ValueError, KeyError and any serialization
exceptions, call logger.exception(...) to log the full traceback and a clear
message, and then fail deterministically (e.g., raise a RuntimeError or call
sys.exit(1)) so the error is explicit rather than an unhandled crash.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
DEVELOPER.md (3)
69-73: Add a language to the fenced code block (markdownlint MD040).Specify a language for the “Expected Output” block to satisfy linters.
-``` +```text All done! ✨ 🍰 ✨ 1 file reformatted.--- `93-98`: **Align mypy example with the preceding flag guidance.** You mention using --check-untyped-defs but the example omits it. ```diff -```shell -mypy src/writer_kafka.py -``` +```shell +mypy src/writer_kafka.py --check-untyped-defs +```
135-137: Use a cross-platform command to open the HTML coverage report.open is macOS-specific; webbrowser works everywhere.
-```shell -open htmlcov/index.html -``` +```shell +python -m webbrowser -t htmlcov/index.html +```README.md (3)
101-101: Fix script name typo (or confirm if intentionally retained).Prefer prepare.deployment.sh for correctness and consistency. If the file in repo still uses the typo, keep docs aligned but consider renaming the file in this PR.
-1. Run packaging script: `scripts/prepare.deplyoment.sh` (downloads deps + zips sources & config) +1. Run packaging script: `scripts/prepare.deployment.sh` (downloads deps + zips sources & config)-- `scripts/prepare.deplyoment.sh` – build Zip artifact for Lambda (typo in name retained for now; may rename later) +- `scripts/prepare.deployment.sh` – build Zip artifact for LambdaAlso applies to: 158-158
152-152: Use the correct AWS API name.Minor wording tweak for accuracy.
-Publishes events to the configured `event_bus_arn` using put events API. +Publishes events to the configured `event_bus_arn` using the PutEvents API.
163-170: Surround the table with blank lines (markdownlint MD058).Add blank lines before and after the Troubleshooting table.
-## Troubleshooting -| Symptom | Possible Cause | Action | +## Troubleshooting + +| Symptom | Possible Cause | Action | @@ -| Lambda keeps old config | Warm container | Call `/terminate` (internal) to force cold start | +| Lambda keeps old config | Warm container | Call `/terminate` (internal) to force cold start | +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DEVELOPER.md(1 hunks)README.md(1 hunks)requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
DEVELOPER.md
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
163-163: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔇 Additional comments (2)
DEVELOPER.md (1)
1-138: Docs look solid and match the new QA tooling.Clear, actionable steps for pylint, Black, mypy, and pytest.
README.md (1)
138-143: Confirm implementation matches security note.Verify that the code indeed fetches the RS256 public key at cold start from token_public_key_url in the documented JSON shape { "key": "..." }.
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
I am fine with, but GitHub do not allow be to approve it as I was part of this PR.
Release Notes:
Closes #54
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores