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

Unify logger.conf and the fallback code #616

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bocekm
Copy link
Member

@bocekm bocekm commented Mar 10, 2020

  • use logging.Formatter.converter = time.gmtime for both the file config and fallback config to have the same (UTC) time, in both cases
  • unify log levels, using the fallback as the ones to use also in the conf file

@bocekm bocekm requested review from vinzenz and jmikovic March 10, 2020 20:12
@centos-ci
Copy link

Can one of the admins verify this patch?

@bocekm bocekm changed the title Add missing leapp logger to logger.conf Unify logger.conf and the fallback code Mar 10, 2020
@bocekm bocekm changed the title Unify logger.conf and the fallback code EDIT: Unify logger.conf and the fallback code Mar 10, 2020
@bocekm bocekm changed the title EDIT: Unify logger.conf and the fallback code WIP: Unify logger.conf and the fallback code Mar 10, 2020
@bocekm bocekm changed the title WIP: Unify logger.conf and the fallback code Unify logger.conf and the fallback code Mar 11, 2020
- use logging.Formatter.converter = time.gmtime for both the file config and fallback
  config to have the same (UTC) time, in both cases
- unify log levels
@leapp-bot
Copy link
Collaborator

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines, pass tests and linter checks before it can be merged.

If you want to re-run tests or request review, you can use following commands as a comment:

  • leapp-ci build to run unit tests and copr build
  • e2e tests to run unit tests, copr build and end-to-end tests (OAMG members only)
  • review please to notify leapp developers of review request

@bocekm bocekm requested review from a team, drehak and zhukovgreen June 24, 2020 08:59
@@ -151,7 +151,8 @@ def run(args, split=False, callback_raw=_console_logging_handler, callback_lineb
"""
Run a command and return its result as a dict.

The execution of the program and it's results are captured by the audit.
The execution of the command and its result is captured by the audit. The CalledProcessError is raised when
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a special docstring syntax for the definition of what being raised:

@@ -151,8 +151,7 @@ def run(args, split=False, callback_raw=_console_logging_handler, callback_lineb
     """
     Run a command and return its result as a dict.
 
-    The execution of the command and its result is captured by the audit. The CalledProcessError is raised when
-    the command exits with non-zero code.
+    The execution of the command and its result is captured by the audit.
 
     :param args: Command to execute
     :type args: list or tuple
@@ -168,6 +167,8 @@ def run(args, split=False, callback_raw=_console_logging_handler, callback_lineb
     :type stdin: int, str
     :return: {'stdout' : stdout, 'stderr': stderr, 'signal': signal, 'exit_code': exit_code, 'pid': pid}
     :rtype: dict
+    :raises:
+        CalledProcessError: if the command exits with non-zero status
     """
     if not args:
         message = 'Command to call is missing.'

datefmt=log_date_format,
stream=sys.stderr,
)
stderr_handler = logging.StreamHandler(sys.stderr)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to put sys.stderr, it is by default

global _logger
if not _logger:
"""
Configure loggers as per the description below.
Copy link
Contributor

Choose a reason for hiding this comment

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

log_format = '%(asctime)s.%(msecs)-3d %(levelname)-8s PID: %(process)d %(name)s: %(message)s'
log_date_format = '%Y-%m-%d %H:%M:%S'
path = os.getenv('LEAPP_LOGGER_CONFIG', '/etc/leapp/logger.conf')
logging.Formatter.converter = time.gmtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't do this. This is a global state of all loggers. What about the one from the cookbook? (https://docs.python.org/3/howto/logging-cookbook.html#formatting-times-using-utc-gmt-via-configuration)
this could be solved something similar to:

diff --git a/etc/leapp/logger.conf b/etc/leapp/logger.conf
index ee1cf91..66102c4 100644
--- a/etc/leapp/logger.conf
+++ b/etc/leapp/logger.conf
@@ -10,7 +10,7 @@ keys=leapp_audit,stream
 [formatter_leapp]
 format=%(asctime)s.%(msecs)-3d %(levelname)-8s PID: %(process)d %(name)s: %(message)s
 datefmt=%Y-%m-%d %H:%M:%S
-class=logging.Formatter
+class=leapp.logger.UTCFormatter
 
 [logger_urllib3]
 level=WARN

global _leapp_logger
if not _leapp_logger:
_leapp_logger = logging.getLogger('leapp')

log_format = '%(asctime)s.%(msecs)-3d %(levelname)-8s PID: %(process)d %(name)s: %(message)s'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think fo ruser would be convininent to see that this time is UTC time

Suggested change
log_format = '%(asctime)s.%(msecs)-3d %(levelname)-8s PID: %(process)d %(name)s: %(message)s'
log_format = '%(asctime)s.%(msecs)-3d UTC %(levelname)-8s PID: %(process)d %(name)s: %(message)s'

MichalHe pushed a commit to MichalHe/leapp that referenced this pull request Aug 12, 2021
In case of docstring, quotes should be used always, officialy.
Replace apostrophes by quotes in case of docstrings.
@fernflower
Copy link
Member

@oamg/developers Do we still want this? I'd rather close and reopen on demand.

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.

5 participants