Skip to content

GSK-2822 Don't edit global logger level#1806

Merged
Hartorn merged 7 commits intomainfrom
GSK-2822
Feb 21, 2024
Merged

GSK-2822 Don't edit global logger level#1806
Hartorn merged 7 commits intomainfrom
GSK-2822

Conversation

@kevinmessiaen
Copy link
Member

Description

Updated logger to only update giskard log level to INFO by default

Related Issue

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

@linear
Copy link

linear bot commented Feb 19, 2024



def test_root_log_level_default_warning():
assert logging.root.level == logging.WARNING, 'Root package log level should be set to default "WARNING"'
Copy link
Member

Choose a reason for hiding this comment

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

I'm all for this modification, but for scan and worker, we should have some logs, no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you're right, it should be an arg for the CLI to allow flexibility

{"is_server": is_server, "url": anonymize(url), "is_daemon": is_daemon, "log_level": log_level},
)

logging.root.setLevel(logging.getLevelName(log_level))
Copy link
Member

Choose a reason for hiding this comment

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

Setting log level does not work for me : if I set it to error, I still have logs from giskard weirdly. Not sure why

Copy link
Member

Choose a reason for hiding this comment

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

Normally setting Error level to root level should suppress every log even from giskard, in my case worker is still starting with logs.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, it was only at start, seems ok after

@Hartorn Hartorn enabled auto-merge (squash) February 21, 2024 09:30
@sonarqubecloud
Copy link

@Hartorn Hartorn merged commit e6112f6 into main Feb 21, 2024
@Hartorn Hartorn deleted the GSK-2822 branch February 21, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants