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

Configure root logger instead of package logger #1773

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

relrod
Copy link
Member

@relrod relrod commented Dec 1, 2021

Change:

  • This was discussed internally; the goal is to make it so that rules
    which set up their own logger are still subject to the global rules
    of ansible-lint particularly with regard to our verbosity/quietness
    flags.

  • To accomodate for using the root logger, the logging levels of -q and
    -qq had to change slightly. This is because the default log level of
    the root logger is WARNING, and we want to avoid setting it to NOTSET
    which would allow everything by default. To work around this, we now
    make our default be WARNING as well. -q becomes ERROR, and -qq
    becomes CRITICAL which we previously didn't have.

Test Plan:

  • New integration tests with various levels of -v and -q.

Signed-off-by: Rick Elrod rick@elrod.me

Change:
- This was discussed internally; the goal is to make it so that rules
  which set up their own logger are still subject to the global rules
  of ansible-lint particularly with regard to our verbosity/quietness
  flags.

- To accomodate for using the root logger, the logging levels of -q and
  -qq had to change slightly. This is because the default log level of
  the root logger is WARNING, and we want to avoid setting it to NOTSET
  which would allow everything by default. To work around this, we now
  make our default be WARNING as well. -q becomes ERROR, and -qq
  becomes CRITICAL which we previously didn't have.

Test Plan:
- New integration tests with various levels of -v and -q.

Signed-off-by: Rick Elrod <rick@elrod.me>
Signed-off-by: Rick Elrod <rick@elrod.me>
Signed-off-by: Rick Elrod <rick@elrod.me>
@relrod relrod added AAP Ansible Automation Platform bug new Triage required patch labels Dec 1, 2021
@relrod relrod marked this pull request as ready for review December 1, 2021 22:58
@relrod relrod requested a review from a team as a code owner December 1, 2021 22:58
@relrod relrod requested a review from ssbarnea December 1, 2021 22:59
@webknjaz
Copy link
Member

webknjaz commented Dec 2, 2021

Affecting an interpreter-global logger seems weird. Why not have a common module with loggers that the rules and other modules could import from?

@relrod
Copy link
Member Author

relrod commented Dec 2, 2021

@webknjaz The primary reason is because people might develop rules that set up their own logger, and this could break apps that parse output (such as navigator or ALS). Configuring the root logger helps prevent that. @ssbarnea might have other thoughts here - I spoke with him in DM a few days ago and he thought this was a good direction to go.

@webknjaz
Copy link
Member

webknjaz commented Dec 2, 2021

@relrod how about providing a way to get a logger in the base class for rules? I think it's worth standardizing on.

@ganeshrn ganeshrn removed the new Triage required label Dec 6, 2021
@ssbarnea ssbarnea merged commit 8c372b6 into ansible:main Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AAP Ansible Automation Platform bug patch
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants