Skip to content

Removed logging configuration#3

Open
ikrivosheev wants to merge 1 commit intoskelsec:mainfrom
ikrivosheev:fix/logging
Open

Removed logging configuration#3
ikrivosheev wants to merge 1 commit intoskelsec:mainfrom
ikrivosheev:fix/logging

Conversation

@ikrivosheev
Copy link

@ikrivosheev
Copy link
Author

@skelsec friendly ping)

@skelsec
Copy link
Owner

skelsec commented Apr 2, 2025

can you please elaborate? there are a few calls to this

@ikrivosheev
Copy link
Author

@skelsec I fixed. Can you review?)

@CravateRouge
Copy link
Contributor

@skelsec to calrify on this, asyauth, minikerberos and msldap all have the same issue.

It is good practice to just call logger = logging.getLogger(__name__) in libraries as it should be to the application to define the handlers, formatters, level, propagation, see official python doc:

Child loggers propagate messages up to the handlers associated with their ancestor loggers. Because of this, it is unnecessary to define and configure handlers for all the loggers an application uses. It is sufficient to configure handlers for a top-level logger and create child loggers as needed. (You can, however, turn off propagation by setting the propagate attribute of a logger to False.)

For the case of msldap and minikerberos which also have a "cli mode" a good solution would be to NOT define the logger option globally as it gives duplicates logs (appearing in the root logger and the library logger) but define the logger options in a function called in the main of each CLI app, e.g.:

import logging
logger = logging.getLogger(__name__)

def enable_cli_logger():
  handler = logging.StreamHandler()
  formatter = logging.Formatter(
          '%(asctime)s %(name)-12s %(levelname)-8s %(message)s')
  handler.setFormatter(formatter)
  logger.addHandler(handler)
  logger.setLevel(logging.INFO)
  logger.propagate = False

def main():
   enable_cli_logger()
    logger.info("Starting CLI tool...")

@ikrivosheev
Copy link
Author

@CravateRouge thank you)

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