Skip to content
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

Fix logger setup #4657

Merged
merged 21 commits into from
Nov 12, 2024
Merged

Fix logger setup #4657

merged 21 commits into from
Nov 12, 2024

Conversation

barryyosi-panw
Copy link
Contributor

Related Issues

fixes: CIAC-12159

Description

When using 'demisto-sdk' as a library within content scripts/integrations, the new logging setup/initialization conflicts with existing logger and causes the container to crash when initialized statically.

Additionally, logs does not show up on default python's logging which is used in content scripts/integration.

Solution:

  • Excluding sdk's logging initialization logic when imported as a library.
  • Propagating log entries to default logging system when flagged.

Copy link

Changelog(s) in markdown:

  • Fixing logger crashes when imported as library in content scripts/integrations. #4657

@barryyosi-panw barryyosi-panw marked this pull request as ready for review November 12, 2024 07:58
Copy link
Contributor

@dorschw dorschw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

"""
global logger
_setup_neo4j_logger()

logger.remove(None) # Removes all pre-existing handlers
logger.remove() # Removes all pre-existing handlers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you sure passing None vs nothing still achieves what we wanted?

Copy link
Contributor Author

@barryyosi-panw barryyosi-panw Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value is None then yes, and it is less confusing that way imo. https://loguru.readthedocs.io/en/stable/api/logger.html#loguru._logger.Logger.remove

.changelog/4657.yml Outdated Show resolved Hide resolved
Co-authored-by: dorschw <81086590+dorschw@users.noreply.github.com>
Copy link

Changelog(s) in markdown:

  • Fixing logger crashes when imported as library. #4657

@barryyosi-panw barryyosi-panw merged commit c82761b into master Nov 12, 2024
25 checks passed
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.

3 participants