Fix cli debugging

[holmanb]

Additional Context

Issue reported by @a-dubs. Please test it if you don't mind. Rebasing your test branch on this one should work, I think.

Test Steps

Tested using this minimal test case:

import logging
from cloudinit import log

log.configure_root_logger()
log.add_log_handler_cli(logging.INFO)
LOG = logging.getLogger()
LOG.warning("hi")

Without this branch, no log is emitted. With this branch, the log appears on stderr,

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[blackboxsw]

Good drop, I think this was originally intended to handle re-entrant calls to setup_basic_logging where stderr stream was already setup to avoid duplication of logs to stderr. I'll double check that this is no longer an issue due to how you've changed logging call-sites in your previous logging refactors.

[blackboxsw]

This looks like it'll mean that if we get multiple calls to setup_basic_logging(<log_level>) then we'll have duplicated logs to stderr for each LOG.root.handler managinghandler. In practice though, the CLI commands we are only calling this once in any case during handling of CLI arguments, so adding the streamhandler for stderr at a given log level will properly configure the default log-level here and I don't think we really need to solve issues with this function not being idempotent.

[TheRealFalcon]

I think it'd be good to have a test that actually verifies that logs go to the cli when using this function. We apparently didn't have one before.

[a-dubs]

Manual testing shows that yes CLI logging is indeed functioning, but I am still unable to get the log message to show up in either stdout or stderr in unit tests via capsys.readouterr().

So yeah as mentioned above, getting unit tests implemented to ensure that logs go to the cli would be useful for catching regressions in the future, but also since I am unable to get my unit tests to see logs appearing in the cli.

[a-dubs]

Commenting out the @mock.patch(M_PATH + "add_log_handler_cli", lambda *args: "") line in test_query.py fixed it! Now your new changes actually get used and my unit test can finally see the error log in stderr! :D

[blackboxsw]

@holmanb great on this, the end result is much cleaner here. And ensuring we setup logging in main instead of down in subcommands also allows is to avoid potential duplication of log StreamHandler setup in subcommands.

One thing I'd suggest prior to merging is squash merging away that first commit as it adds the log_to_stderr that you eventually drop in the last commit. in favor of a better setup in cloudinit.cmd.main.

[blackboxsw]

Leaving in approved under expectation that you will do something with the commits/squashes to better represent the dropped addLogHandlerCLI from the first commit and the the final feat commit to represent where that setup_basic_logging is now located.

[holmanb]

One thing I'd suggest prior to merging is squash merging away that first commit as it adds the log_to_stderr that you eventually drop in the last commit. in favor of a better setup in cloudinit.cmd.main.

Agreed, I should have mentioned that I left those commits separate in case there was objection to the optional ones that aren't needed for 23.4. I'll squash away the first refactor commit since that is redundant with removing the call entirely and shifting logic into main.py.