logging: no more cli console logging #2683
Conversation
orndorffgrant
force-pushed
the
no-more-console-logging
branch
from
ff61ceb
to
063d74a
Compare
|
Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference) GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references) Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references) Documentation: The changes in this PR do not require documentation changes. 👍 this comment to confirm that this is correct. |
orndorffgrant
force-pushed
the
no-more-console-logging
branch
from
063d74a
to
3529d12
Compare
orndorffgrant
force-pushed
the
no-more-console-logging
branch
from
3529d12
to
41f6abd
Compare
orndorffgrant
changed the title
orndorffgrant
force-pushed
the
no-more-console-logging
branch
from
41f6abd
to
242ad4e
Compare
uaclient/cli.py
Outdated
| LOG.warning(NEW_VERSION_NOTICE.format(version=new_version)) | ||
| print(msg, file=sys.stderr) |
There was a problem hiding this comment.
Shouldn't this be a event.info call ?
There was a problem hiding this comment.
Yep it probably should, and same for the others, updating
uaclient/cli.py
Outdated
| ) | ||
| LOG.warning(msg) | ||
| print(msg, file=sys.stderr) |
There was a problem hiding this comment.
Same event.info replacement here
| @@ -10,22 +10,27 @@ | |||
| import yaml | |||
| except ImportError: | |||
| LOG.error(MISSING_YAML_MODULE.msg) | |||
| print(MISSING_YAML_MODULE.msg, file=sys.stderr) | |||
There was a problem hiding this comment.
I think it should be fine to use print, but I just want to double check if we should change to event.info as well
There was a problem hiding this comment.
I'm leaving these as prints because they only happen in really broken scenarios, and importing event_logger is a circular dependency
orndorffgrant
force-pushed
the
no-more-console-logging
branch
from
242ad4e
to
4df6b40
Compare
|
@lucasmoura updated! |
orndorffgrant
force-pushed
the
no-more-console-logging
branch
from
4df6b40
to
735e7b7
Compare
Why is this needed?
This removes some old code around conditionally logging messages to the console. In most cases, that is what we do, and in some of the few cases where we didn't disable the console logger, that was a bug.
We should treat the console as our cli UX and treat the logs as logs. There should be no instance where a log message is also part of our UX. In the rare case where we want to print the same message to the User and our logs, then that can be two separate statements.
The exception is the
--debugflag which is useful to include log statements in the middle of the console output - that functionality is kept and enhanced by removing the formatter that stripped a lot of useful information from those logs.Test Steps
CI should pass.
Please review all instances of logging.info, logging.warn, logging.error, and logging.exception. Please double check that, where necessary, the messages are also printed/event.infoed properly. This includes statements that are not a part of the diff.
Checklist
Does this PR require extra reviews?