Skip to content

Conversation

@haroldrandom
Copy link
Contributor

@haroldrandom haroldrandom commented Apr 10, 2020

Description of PR (Mandatory)
I am migrating test framwork from nose to pytest and found CLI would log every command's invocation output to a filep even though I configured in ~/.azure/config

[logging]
enable_log_file = no

And consequently, pytest wouldn't clear the AzCLI instance cleanly such that the test process would crash because of holding too many file descriptor (over the limit by OS):
image
image

So, here, I want to enable the ability to repect the logging configuratin even though I won't use it in CI. I will fire another PR to fix the bug that CLI holding too many opened files. (related issues: #12882, #10435)

Testing Guide
It affects two instance variables' assignment command_logger_handler and command_metadata_logger.
In the places they are referenced, there is a fault protection, so, it won't be a problem if user disbale the output of a logger to a log file.

Verify by running command with prefix AZURE_LOGGING_ENABLE_LOG_FILE=no or config in ~/.azure/config like above, then there should be no files under ~/.azure/commands/. Otherwise, there should be a file under that folder

History Notes:
(Fill in the following template if multiple notes are needed, otherwise PR title will be used for history note.)

[Component Name 1] (BREAKING CHANGE:) (az command:) make some customer-facing change.
[Component Name 2] (BREAKING CHANGE:) (az command:) make some customer-facing change.


This checklist is used to make sure that common guidelines for a pull request are followed.

@haroldrandom haroldrandom added the Core CLI core infrastructure label Apr 10, 2020
@haroldrandom haroldrandom self-assigned this Apr 10, 2020
@haroldrandom haroldrandom changed the title {Core} Make CLI respect file logging configuration {Core} Make CLI respect file logging configuration: enable_log_file Apr 10, 2020
@haroldrandom haroldrandom changed the title {Core} Make CLI respect file logging configuration: enable_log_file {Core} Make CLI respect file logging configuration option: enable_log_file Apr 10, 2020
@haroldrandom haroldrandom marked this pull request as ready for review April 10, 2020 05:38
@haroldrandom haroldrandom marked this pull request as draft April 10, 2020 06:15
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 10, 2020

pytest

@yonzhan yonzhan added this to the S168 milestone Apr 10, 2020
self._init_command_logfile_handlers(cmd_logger, args) # pylint: disable=protected-access
get_logger(__name__).debug("metadata file logging enabled - writing logs to '%s'.", self.command_log_dir)
# overwrite CLILogging._is_file_log_enabled() from knack
self.file_log_enabled = cli_ctx.config.getboolean('logging', 'enable_log_file', fallback=True)
Copy link
Member

Choose a reason for hiding this comment

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

The default for logging is no in az configure.

Copy link
Member

Choose a reason for hiding this comment

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

However, changing it to no could be a breaking change since the bug made the actual default to be logging enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I overwrite the implementation from knack to enable logging by default like currenty CLI does, so it's transparency to user. Not a breaking it is.

@haroldrandom haroldrandom requested a review from mmyyrroonn April 10, 2020 07:32
@haroldrandom haroldrandom marked this pull request as ready for review April 10, 2020 07:46
Copy link
Member

@jiasli jiasli left a comment

Choose a reason for hiding this comment

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

FYI, Knack handles the logging correctly:

https://github.com/microsoft/knack/blob/master/knack/log.py#L129-L131

        if self.file_log_enabled:
            self._init_logfile_handlers(root_logger, cli_logger)
            get_logger(__name__).debug("File logging enabled - writing logs to '%s'.", self.log_dir)

@haroldrandom haroldrandom changed the title {Core} Make CLI respect file logging configuration option: enable_log_file {Core} Make CLI respect file logging configuration option: enable_log_file and AZURE_LOGGING_ENABLE_LOG_FILE env Apr 10, 2020
@haroldrandom haroldrandom changed the title {Core} Make CLI respect file logging configuration option: enable_log_file and AZURE_LOGGING_ENABLE_LOG_FILE env {Core} Make CLI respect file logging configuration option: enable_log_file and AZURE_LOGGING_ENABLE_LOG_FILE env variable Apr 10, 2020
@haroldrandom haroldrandom merged commit ec56c66 into Azure:dev Apr 10, 2020
@haroldrandom haroldrandom deleted the cli-logging-config branch April 10, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core CLI core infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants