Skip to content

fix(logging): isolate CLI logging setup to avoid affecting library users #5022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JigyasuRajput
Copy link
Contributor

Description:

Fixes #4559
This PR refactors the logging setup in cve-bin-tool to prevent unintended logging configuration when the package is used as a library. Specifically:

  • Added NullHandler to the package logger in __init__.py to avoid “No handler found” warnings.
  • Disabled logger propagation to prevent logs from bubbling up to the root logger.
  • Removed default Rich logging setup from log.py.
  • Introduced setup_rich_logger() to be explicitly called in CLI context.
  • Updated cli.py to call setup_rich_logger() during CLI entry point execution.
  • Added test_logging.py to validate that:
    • Package logger has a NullHandler
    • Root logger remains unmodified
    • No propagation occurs
    • setup_rich_logger() correctly adds a RichHandler

@JigyasuRajput JigyasuRajput marked this pull request as draft April 15, 2025 17:35
@joydeep049
Copy link
Contributor

Hi @JigyasuRajput
I see that you have marked this PR as draft. Are you waiting for something to get merged first?

Also, feel free to have the CI re-run, since the CI issues have already been fixed.

@JigyasuRajput
Copy link
Contributor Author

JigyasuRajput commented Apr 29, 2025

Hi @JigyasuRajput I see that you have marked this PR as draft. Are you waiting for something to get merged first?

Also, feel free to have the CI re-run, since the CI issues have already been fixed.

hey!
I marked it as a draft because "it was not ready for a review yet", I still might need to tweak some changes so the tests in the CI do pass, but thanks for the heads-up!
I'm still working on my other PR to get them merged (specially the issue we have with uc\ /lib) I'll soon update this branch with more changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: (logging) Why do you add the RichHandler to the root logger?
2 participants