Support -o log_cli=<true/false> to turn off console logging#884
Support -o log_cli=<true/false> to turn off console logging#884dbasunag merged 5 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/verified', '/hold', '/wip', '/cherry-pick', '/lgtm'} |
📝 WalkthroughWalkthroughRestructures test logging initialization: logging is configured earlier with resolved Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
utilities/logger.py (2)
25-27: Update docstring for new parameter.The function signature now accepts
enable_consolebut the docstring doesn't document it. Additionally, lines 75-77 suggestlog_levelcan be a string, but the docstring says it's an int.Apply this diff to update the docstring:
def setup_logging( log_level: int, log_file: str = "/tmp/pytest-tests.log", thread_name: str | None = None, enable_console: bool = True ) -> QueueListener: """ Setup basic/root logging using QueueHandler/QueueListener to consolidate log messages into a single stream to be written to multiple outputs. Args: - log_level (int): log level + log_level (int | str): log level (int constant or string name like 'INFO') log_file (str): logging output file thread_name (str | None): optional thread_name id prefix, e.g., [gw0] + enable_console (bool): whether to enable console logging (default: True) Returns: QueueListener: Process monitoring the log Queue
70-72: Document the PYTEST_DISABLE_CONSOLE_LOG environment variable.This environment variable provides an alternative way to disable console logging but isn't mentioned in the docstring or any user-facing documentation in this PR.
Consider adding a note to the docstring about this environment override.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conftest.py(1 hunks)utilities/logger.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
utilities/logger.py
26-26: Probable insecure usage of temporary file or directory: "/tmp/pytest-tests.log"
(S108)
conftest.py
311-311: Do not catch blind exception: Exception
(BLE001)
313-313: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (6)
conftest.py (4)
284-293: LGTM! Clean log level conversion.The string-to-int log level conversion is correctly implemented with appropriate fallback to
logging.INFO.
295-297: LGTM! Proper cleanup sequence.Removing the old log file before initializing logging is the correct approach.
317-322: LGTM! Proper logging initialization.The
setup_loggingcall correctly passes all computed parameters.
324-328: LGTM! Post-configuration logging is safe.Logging after
setup_loggingcompletes is the correct approach, as noted by the comment.utilities/logger.py (2)
83-84: LGTM! Dynamic handler composition.Using
*handlersto unpack the dynamic handler list is the right approach for conditional console logging.
108-126: LGTM! Comprehensive logger reconfiguration.The thorough clearing and reconfiguration of all loggers ensures consistent queue-based logging behavior. The redundant handler clearing (lines 118 and 123-125) is defensive but harmless.
jgarciao
left a comment
jgarciao
left a comment
There was a problem hiding this comment.
Could you add a note in GETTING_STARTED.md explaining how to use this parameter?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
utilities/logger.py (1)
64-72: Movelog_levelnormalization before anysetLevelcalls (still applied too late).
log_file_handler.setLevel(level=log_level)on Line 65 can still receive a lowercase string (e.g."debug"), while you only normalize/uppercaselog_levelafterwards (Lines 69‑71). In that case,Handler.setLevelwill raiseValueError("Unknown level")before your normalization runs.This is the same issue noted in the earlier review; the conversion needs to happen once, up front, before any handler/logger
setLevelcalls.Suggested change:
- Add normalization near the top of
setup_logging(before anysetLevelusage), e.g.:# Normalize log_level to an int once, early if isinstance(log_level, str): log_level = getattr(logging, log_level.upper(), logging.INFO)
- Remove the late conversion in this block:
- # Convert log_level to int if it's a string - if isinstance(log_level, str): - log_level = getattr(logging, log_level.upper(), logging.INFO)This way all handlers/loggers consistently see a valid integer level and lowercase strings work as intended.
🧹 Nitpick comments (4)
utilities/logger.py (4)
5-5: Tighten type hints for handlers instead of usingAny.
handlersis always a list of logging handlers, soAnyis broader than needed. You can keep the code self-documenting without extra imports:-from typing import Optional, Any +from typing import Optional @@ - handlers: list[Any] = [log_file_handler] + handlers: list[logging.Handler] = [log_file_handler]This avoids
Anywhile reflecting the actual type.Also applies to: 67-67
24-37: Alignsetup_loggingsignature and docstring with actual behavior (log_level + enable_console).Two small consistency issues here:
log_levelis annotated and documented asint, but the function explicitly accepts strings and normalizes them later.- The new
enable_consoleparameter isn’t documented in theArgssection.Consider updating both signature and docstring, e.g.:
def setup_logging( log_level: int | str, log_file: str = "/tmp/pytest-tests.log", thread_name: str | None = None, enable_console: bool = True, ) -> QueueListener: """ Args: log_level (int | str): Log level (e.g. logging.INFO or "info"/"INFO"). log_file (str): Logging output file. thread_name (str | None): Optional thread_name id prefix, e.g., [gw0]. enable_console (bool): Whether to attach a console (stream) handler. """
25-25: Reconsider hard‑coding/tmp/pytest-tests.logas the default log file.Ruff’s S108 warning is about potential issues with world‑writable temp dirs. Even if this is “just tests”, you might want a slightly safer/more flexible default, e.g.:
- Use
tempfile.gettempdir()orPath(tempfile.gettempdir()) / "pytest-tests.log".- Or default to a repository‑local path and make
/tmpopt‑in via config/ENV.Not urgent, but worth a follow‑up if these logs can contain sensitive data or run on shared hosts.
103-121: Central re‑binding of all loggers to the queue handler may override third‑party logging configs.The new block:
- Iterates over
logging.root.manager.loggerDictand clears/rewires every logger (except"root"and"basic") toroot_log_queue_handlerwithpropagate = False.- Resets
logging.rootto only useroot_log_queue_handler.This guarantees everything flows through your queue + console/file policy, which is nice for tests, but it also:
- Discards any handlers/filters third‑party libraries may have attached.
- Changes
propagatesemantics for existing loggers.If that’s intentional for this test harness, document it clearly where
setup_loggingis called; otherwise, you may want a narrower scope (e.g., only re‑bind known logger namespaces, or skip loggers that already use your queue handler). Also note Lines 117‑120 are effectively redundant right afterhandlers.clear()+ singleaddHandler, so they can be trimmed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/logger.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.6)
utilities/logger.py
25-25: Probable insecure usage of temporary file or directory: "/tmp/pytest-tests.log"
(S108)
🔇 Additional comments (1)
utilities/logger.py (1)
73-76: Console toggle viaenable_consoleis straightforward and matches the PR goal.Conditionally creating the
StreamHandlerand wiring it into theQueueListenerbased solely onenable_consolegives a clear, single control point for console logging. This aligns well with the-o log_cli=<true/false>behavior described in the PR.
|
Status of building tag latest: success. |
…hub-io#884) * Support -o log_cli=<true/false> to turn off console logging * Add information on how to disable console logging * Removed support for PYTEST_DISABLE_CONSOLE_LOG as not necessary
5 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.