FIX(config) Respect log_level from settings.yaml and suppress premature debug output#9
Merged
FIX(config) Respect log_level from settings.yaml and suppress premature debug output#9
Conversation
…re debug output Settings.yaml log_level was ignored because config was loaded after --log-level argument default was set. Additionally, debug messages from config loading appeared before the logger was properly configured. Changes: - Load config defaults before creating argparse arguments - Use config file log_level as default instead of hardcoded "INFO" - Remove loguru default handler before parsing args to suppress early debug output - Add log_config_status() to log config info AFTER logger is configured - Remove redundant --log-level from trigger/summary subcommands - Note: --log-level must now come BEFORE subcommand (e.g., rptool --log-level DEBUG write) Fixes: #6 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
📝 WalkthroughWalkthroughTop-level CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "User / CLI"
participant Dispatcher as "rp_dispatcher"
participant Parser as "ap.create_main_parser"
participant Config as "config.load_config_file"
participant Log as "Loguru / stderr handler"
CLI->>Dispatcher: invoke command
Dispatcher->>Log: remove default handler
Dispatcher->>Log: add temporary stderr handler (level from env/config)
Dispatcher->>Parser: call create_main_parser()
Parser->>Config: _get_config_defaults() / load_config_file()
Config-->>Parser: return defaults (or raise ValueError)
alt config valid
Parser-->>Dispatcher: return parser
Dispatcher->>CLI: parse args
Dispatcher->>Log: remove temp handlers
Dispatcher->>Log: add stderr handler (level=args.log_level)
Dispatcher->>Dispatcher: run command
else config invalid
Parser-->>Dispatcher: raises ValueError
Dispatcher->>Log: log error
Dispatcher-->>CLI: exit code 1
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/reportportal/ap.py`:
- Around line 65-69: Normalize and validate the configured log level before
using it as the argparse default: ensure defaults["log_level"] (and any
LOG_LEVEL env/config value) is converted to upper() and checked against the
allowed choices ["DEBUG","INFO","WARNING","ERROR"], falling back to "INFO" if
invalid, then pass that validated value into parser.add_argument default; also
ensure the value passed to logger.add(level=...) is the validated, uppercased
string to avoid Loguru ValueError.
In `@tests/unit/test_ap.py`:
- Around line 611-672: Wrap the calls to reportportal.rp_dispatcher.main(...) in
a try/finally inside both test_no_premature_debug_messages_during_config_load
and test_config_status_logged_with_debug_level so that the Loguru global logger
is cleaned up after each test; in the finally block import logger from loguru
and call logger.remove() (and any other per-test restoration if you added sinks)
to ensure the global logger state added by main() is cleared and does not leak
into subsequent tests.
In `@tests/unit/test_config.py`:
- Around line 295-348: Both tests
(TestLogConfigStatus.test_log_config_status_file_not_found and
TestLogConfigStatus.test_log_config_status_empty_file) mutate the global Loguru
logger via logger.remove() and logger.add(...); update each test to save and
restore the original logger state around the assertions (use try/finally).
Specifically, before calling logger.remove(), capture the current handlers/state
(so you can restore it), then run the test logic that calls log_config_status(),
and in the finally block restore the original Loguru configuration (re-add the
saved handlers or reset to the saved state) so other tests are not affected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22105c1d-7f52-4742-aa8a-5f2fb714c3b6
📒 Files selected for processing (5)
src/reportportal/ap.pysrc/reportportal/config.pysrc/reportportal/rp_dispatcher.pytests/unit/test_ap.pytests/unit/test_config.py
silvi-t
reviewed
Apr 14, 2026
…premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
…ppress premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
… and suppress premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Contributor
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
silvi-t
reviewed
Apr 17, 2026
…gs.yaml and suppress premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/reportportal/ap.py (1)
65-76: Log-level validation looks good; minor UX note on failure mode.The uppercase normalisation plus membership check correctly prevents invalid values from
settings.yaml/LOG_LEVELreachinglogger.add(level=...). One small caveat: because this runs unconditionally insidecreate_main_parser(), an invalidlog_levelin the config will also breakrptool --help/rptool --version, not just real invocations. If that matters, consider deferring validation until afterparse_args(or falling back toINFOwith alogger.warning) rather than raising. Otherwise LGTM — the dispatcher'sValueErrorhandler makes this acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reportportal/ap.py` around lines 65 - 76, The current unconditional validation of defaults.get("log_level") in create_main_parser() (checking configured_log_level against valid_log_levels) causes an exception during simple invocations like rptool --help or --version; instead defer or soften validation: remove the hard raise in create_main_parser(), keep configured_log_level but if it's invalid set parser's default to "INFO" and emit a runtime warning (via logger.warning) after args are parsed in the main dispatcher, or alternatively validate configured_log_level after parse_args and raise/handle there; update references to configured_log_level, valid_log_levels, and parser.add_argument to implement the deferred validation/fallback approach so help/version remain usable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/reportportal/config.py`:
- Around line 57-60: The except block currently logs with Loguru formatting but
raises ValueError using unused `{}` placeholders and loses the original
traceback; update the except around reading the config to catch only concrete
errors (e.g., OSError and yaml.YAMLError), raise a ValueError with a properly
formatted message (using f-string or .format to inject config_file and e) and
chain the original exception with `from e`; reference the logger and config_file
variables and the ValueError raised in the except block so the change is applied
to that handler.
---
Nitpick comments:
In `@src/reportportal/ap.py`:
- Around line 65-76: The current unconditional validation of
defaults.get("log_level") in create_main_parser() (checking configured_log_level
against valid_log_levels) causes an exception during simple invocations like
rptool --help or --version; instead defer or soften validation: remove the hard
raise in create_main_parser(), keep configured_log_level but if it's invalid set
parser's default to "INFO" and emit a runtime warning (via logger.warning) after
args are parsed in the main dispatcher, or alternatively validate
configured_log_level after parse_args and raise/handle there; update references
to configured_log_level, valid_log_levels, and parser.add_argument to implement
the deferred validation/fallback approach so help/version remain usable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc667a06-3a04-443a-aac8-d2bf5c000805
📒 Files selected for processing (5)
src/reportportal/ap.pysrc/reportportal/config.pysrc/reportportal/rp_dispatcher.pytests/unit/test_ap.pytests/unit/test_config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/reportportal/rp_dispatcher.py
- tests/unit/test_ap.py
- tests/unit/test_config.py
… settings.yaml and suppress premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
…el from settings.yaml and suppress premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Contributor
Author
|
Changes:
|
silvi-t
reviewed
Apr 17, 2026
…log_level from settings.yaml and suppress premature debug output Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/reportportal/config.py (1)
60-62:⚠️ Potential issue | 🟡 MinorChain the raised
ValueErrorand narrow theexcept.Same findings as the prior round:
raise ValueError(...)inside anexceptshould usefrom e(Ruff B904) to preserve the original traceback, and the blindexcept Exception(Ruff BLE001) should be narrowed to the concrete errors actually thrown byopen()/yaml.safe_load().🛠️ Proposed fix
- except Exception as e: - # need to raise ValueError to indicate critical problem - raise ValueError(f"Error reading config file {config_file} {e}") + except (OSError, yaml.YAMLError) as e: + # need to raise ValueError to indicate critical problem + raise ValueError(f"Error reading config file {config_file}: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reportportal/config.py` around lines 60 - 62, The except block catching Exception when reading the config file should be narrowed to the specific errors thrown by open() and yaml.safe_load() (e.g., FileNotFoundError, PermissionError, OSError, and yaml.YAMLError) instead of a blind except, and when re-raising wrap the new ValueError with the original exception using "from e" so the traceback is preserved; update the handler around the code that uses open(config_file, ...) and yaml.safe_load(...) to catch those concrete exceptions and change "raise ValueError(f'Error reading config file {config_file} {e}')" to raise ValueError(...) from e.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/reportportal/config.py`:
- Around line 60-62: The except block catching Exception when reading the config
file should be narrowed to the specific errors thrown by open() and
yaml.safe_load() (e.g., FileNotFoundError, PermissionError, OSError, and
yaml.YAMLError) instead of a blind except, and when re-raising wrap the new
ValueError with the original exception using "from e" so the traceback is
preserved; update the handler around the code that uses open(config_file, ...)
and yaml.safe_load(...) to catch those concrete exceptions and change "raise
ValueError(f'Error reading config file {config_file} {e}')" to raise
ValueError(...) from e.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b249e02-1362-45db-8dab-855f8b5c1fe8
📒 Files selected for processing (3)
src/reportportal/config.pysrc/reportportal/rp_dispatcher.pytests/unit/test_rp_dispatcher_write.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/reportportal/rp_dispatcher.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Settings.yaml log_level was ignored because config was loaded after --log-level argument default was set. Additionally, debug messages from config loading appeared before the logger was properly configured.
Changes:
Fixes: #6
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Signed-off-by: Zdenek Kraus zkraus@redhat.com
Summary by CodeRabbit
Bug Fixes
Behaviour Changes
Tests