Add configuration directory resolution and update event_gate_lambda to use it #57
Conversation
oto-macenauer-absa
requested review from
ABMC831,
OlivieFranklova,
Zejnilovic and
petr-pokorny-absa
as code owners
|
Warning Rate limit exceeded@oto-macenauer-absa has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a PR workflow input for release-notes title matching. Introduces a new configuration directory resolver module and integrates it into the Lambda. Updates message posting flow to perform per-topic authorization, aggregate writer results, and refine token extraction. Writer modules receive formatting updates; EventBridge gains an early no-op return; Postgres uses parameterized SQL. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API Gateway
participant Lambda as event_gate_lambda
participant Conf as conf_path
participant Schema as Schema Validator
participant EB as writer_eventbridge
participant K as writer_kafka
participant PG as writer_postgres
rect rgb(240,245,255)
note over Lambda,Conf: Startup / cold start
Lambda->>Conf: resolve_conf_dir()
Conf-->>Lambda: CONF_DIR, INVALID_CONF_ENV
Lambda->>Lambda: Load config, topics, schemas
end
Client->>API: POST /topics/{topicName}
API->>Lambda: event (body, headers)
Lambda->>Lambda: extract_token(event)
Lambda->>Lambda: decode/verify JWT
Lambda->>Lambda: authorize user ∈ ACCESS[topicName]?
alt unauthorized
Lambda-->>API: 403 error (not member)
API-->>Client: 403
else authorized
Lambda->>Schema: validate(message, topic schema)
alt invalid
Lambda-->>API: 400 error (validation)
API-->>Client: 400
else valid
par write in parallel
Lambda->>EB: write(topicName, message)
Lambda->>K: write(topicName, message)
Lambda->>PG: write(topicName, message)
end
Lambda->>Lambda: collect results
alt any failure
Lambda-->>API: 500 with aggregated writer errors
API-->>Client: 500
else all ok
Lambda-->>API: 202 accepted
API-->>Client: 202
end
end
end
sequenceDiagram
autonumber
participant Env as Environment
participant FS as Filesystem
participant Conf as conf_path.resolve_conf_dir
Env-->>Conf: read env[CONF_DIR]
alt env path exists and is dir
Conf-->>Conf: use env dir
else env set but invalid
Conf-->>Conf: mark invalid_env = abs(path)
FS-->>Conf: check <project_root>/conf
alt exists
Conf-->>Conf: use project conf
else
FS-->>Conf: check <module_dir>/conf
alt exists
Conf-->>Conf: use module conf
else
Conf-->>Conf: fallback to <project_root>/conf
end
end
end
Conf-->>Caller: (conf_dir, invalid_env)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/writer_postgres.py (1)
209-246: Broadened except hides root cause; log stacktrace and fix message formatting.Use logger.exception and {!s} conversion; also the current text has a grammatical issue.
- except Exception as e: - err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + except Exception as e: + _logger.exception("The Postgres writer failed with an unexpected error") + return False, f"The Postgres writer failed with an unexpected error: {e!s}"
🧹 Nitpick comments (15)
src/writer_kafka.py (3)
41-43: Silence the unused lambda arg warning in delivery callback.Rename the unused
msgparameter to_to satisfy linters without changing behavior.- callback=lambda err, msg: ( + callback=lambda err, _: ( errors.append(str(err)) if err is not None else None ),
39-41: Prefer no key over empty-string key.Passing
key=Noneis clearer and avoids potential type issues across client versions.- key="", + key=None,
50-53: Log the stacktrace and use explicit conversion in the message.Use
logger.exceptionand{e!s}to satisfy linters and aid troubleshooting.- 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!s}" + _logger.exception(err_msg) + return False, err_msg.github/workflows/check_pr_release_notes.yml (1)
18-24: Pin release-notes-presence-check action and scope GITHUB_TOKEN permissions
- v0.2.1 supports the
titleinput and treats it as a regex (default:[Rr]elease [Nn]otes:).- Add a
permissions:block to grant onlycontents: readandpull-requests: read.- Pin
AbsaOSS/release-notes-presence-checkto a full commit SHA for supply-chain safety.src/writer_eventbridge.py (1)
39-42: Log stacktrace and use explicit conversion.Switch to
exceptionand{e!s}to improve diagnostics and satisfy lint.- except Exception as e: - err_msg = f"The EventBridge writer failed with unknown error: {str(e)}" - _logger.error(err_msg) - return False, err_msg + except Exception as e: + err_msg = f"The EventBridge writer failed with unknown error: {e!s}" + _logger.exception(err_msg) + return False, err_msgsrc/conf_path.py (2)
26-55: Add docstring and typing for clarity.A brief docstring and return typing make the contract explicit.
-def resolve_conf_dir(env_var: str = "CONF_DIR"): +from typing import Optional, Tuple + +def resolve_conf_dir(env_var: str = "CONF_DIR") -> Tuple[str, Optional[str]]: + """ + Resolve the configuration directory. + Order: + 1) env var if it points to an existing directory + 2) <project_root>/conf + 3) <this_module_dir>/conf + 4) fallback to <project_root>/conf (may not exist) + Returns: (conf_dir, invalid_env_path_or_None) + """
57-59: Sort and freeze public API list.Sort
__all__(lint) and consider a tuple to signal immutability.-CONF_DIR, INVALID_CONF_ENV = resolve_conf_dir() - -__all__ = ["resolve_conf_dir", "CONF_DIR", "INVALID_CONF_ENV"] +CONF_DIR, INVALID_CONF_ENV = resolve_conf_dir() + +__all__ = ("CONF_DIR", "INVALID_CONF_ENV", "resolve_conf_dir")src/writer_postgres.py (3)
8-10: Remove unnecessary noqa and rely on actual usage of psycopg2.Ruff flags the unused noqa directive. The import is used later, so the pragma isn’t needed.
- import psycopg2 # noqa: F401 + import psycopg2
36-71: SQL values are parameterized (good); table name still interpolated.Given tables are chosen from fixed mappings in write(), this is safe in practice. If you ever pass user-controlled identifiers, validate against a whitelist or use psycopg2.sql.Identifier.
Do you want a patch to add a small whitelist assertion here?
218-224: Add a connection timeout to avoid hanging connections.Prevents cold-start stalls on unreachable DB.
- with psycopg2.connect( + with psycopg2.connect( database=POSTGRES["database"], host=POSTGRES["host"], user=POSTGRES["user"], password=POSTGRES["password"], - port=POSTGRES["port"], + port=POSTGRES["port"], + connect_timeout=int(os.environ.get("POSTGRES_CONNECT_TIMEOUT", "5")), ) as connection:src/event_gate_lambda.py (5)
77-79: Avoid disabling TLS verification globally for S3.Security hardening: rely on default verification unless you have a specific reason.
-aws_s3 = boto3.Session().resource("s3", verify=False) +aws_s3 = boto3.Session().resource("s3")
23-26: Import InvalidTokenError to narrow JWT exception handling.Preps for the decode change below.
import boto3 import jwt +from jwt.exceptions import InvalidTokenError import requests
154-160: Narrow JWT error handling and guard for missing public key.Avoids masking non-auth errors and returns a clear 503 if PK fetch failed.
def post_topic_message(topicName, topicMessage, tokenEncoded): logger.debug(f"Handling POST {topicName}") - try: - token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) - except Exception: - return _error_response(401, "auth", "Invalid or missing token") + if not TOKEN_PUBLIC_KEY: + return _error_response(503, "auth", "Token verification unavailable") + try: + token = jwt.decode(tokenEncoded, TOKEN_PUBLIC_KEY, algorithms=["RS256"]) + except InvalidTokenError: + return _error_response(401, "auth", "Invalid or missing token")
200-211: Trim tokens and accept lowercase ‘authorization’.Minor robustness improvements.
- if "bearer" in eventHeaders: - return eventHeaders["bearer"] + if "bearer" in eventHeaders: + return eventHeaders["bearer"].strip() - if "Authorization" in eventHeaders and eventHeaders["Authorization"].startswith( - "Bearer " - ): - return eventHeaders["Authorization"][len("Bearer ") :] + auth = eventHeaders.get("Authorization") or eventHeaders.get("authorization") + if isinstance(auth, str) and auth.lower().startswith("bearer "): + return auth[len("Bearer ") :].strip() return "" # Will result in 401
234-235: Log full stacktrace on unexpected exceptions.Aids troubleshooting; aligns with TRY400.
- logger.error(f"Unexpected exception: {e}") + logger.exception("Unexpected exception")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/check_pr_release_notes.yml(1 hunks)src/conf_path.py(1 hunks)src/event_gate_lambda.py(6 hunks)src/writer_eventbridge.py(3 hunks)src/writer_kafka.py(2 hunks)src/writer_postgres.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/writer_kafka.py (3)
src/writer_eventbridge.py (1)
init(6-15)src/writer_postgres.py (1)
init(13-33)tests/test_event_gate_lambda.py (1)
flush(19-20)
src/writer_eventbridge.py (1)
src/writer_postgres.py (1)
init(13-33)
src/event_gate_lambda.py (4)
tests/test_event_gate_lambda.py (4)
read(39-44)Bucket(52-53)Object(49-50)get(46-47)src/writer_eventbridge.py (1)
init(6-15)src/writer_kafka.py (1)
init(7-30)src/writer_postgres.py (1)
init(13-33)
src/writer_postgres.py (1)
tests/test_writer_postgres.py (1)
execute(11-12)
🪛 Ruff (0.12.2)
src/conf_path.py
59-59: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
src/writer_kafka.py
41-41: Unused lambda argument: msg
(ARG005)
50-50: Do not catch blind exception: Exception
(BLE001)
51-51: Use explicit conversion flag
Replace with conversion flag
(RUF010)
52-52: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
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/event_gate_lambda.py
33-33: Do not catch blind exception: Exception
(BLE001)
35-35: Redefinition of unused conf_path from line 30
(F811)
97-97: Probable use of requests call without timeout
(S113)
98-98: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
158-158: Do not catch blind exception: Exception
(BLE001)
src/writer_postgres.py
8-8: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
240-240: Do not catch blind exception: Exception
(BLE001)
241-241: Use explicit conversion flag
Replace with conversion flag
(RUF010)
242-242: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (2)
src/writer_eventbridge.py (2)
19-21: Early no-op return: confirm desired semantics.Returning success when
EVENT_BUS_ARNis unset skips EventBridge delivery entirely. Confirm the orchestrator treats this writer as optional.
28-32: Validate EventBus identifier form.
put_eventsusesEventBusName; ensureEVENT_BUS_ARNholds a compatible value (name or ARN per SDK semantics) to avoid silent routing issues.
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
This pull request introduces a reusable configuration directory resolution module and refactors the codebase to use it, resulting in improved configuration handling and cleaner code. Additionally, it standardizes code formatting and enhances logging for better debugging. Below are the most important changes:
Configuration Handling Improvements:
conf_path.pythat provides a reusable functionresolve_conf_dirto determine the configuration directory based on environment variables and fallback logic. This centralizes and simplifies configuration directory resolution.event_gate_lambda.pyto use the newconf_pathmodule for configuration directory resolution, removing duplicated logic and improving maintainability.event_gate_lambda.pyto report which configuration directory is being used and to warn if theCONF_DIRenvironment variable is set incorrectly.Code Formatting and Consistency:
event_gate_lambda.py,writer_eventbridge.py,writer_kafka.py, andwriter_postgres.pyfor consistency and readability. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]Workflow Update:
Release notes
Related
Summary by CodeRabbit