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

Add new logging config for CLI hooks manager #1058

Merged
merged 4 commits into from
Nov 23, 2021

Conversation

antonymilne
Copy link
Contributor

Description

In #1024, someone thought it was a good idea to tidy up the loggers a bit. Unfortunately, what said person didn't realise is that this resulted in an incredibly annoying message every time you execute a kedro command outside a project...
image

The reason for this is that the CLI hooks manager was trying to write to a file logs/info.log, which didn't exist. Actually if we're not in a kedro project we shouldn't be trying to log messages to a file at all because that would be really annoying. I've added a special logging handler for this case which means that the message will only be shown on the console and not written to any file.

The other instances where I had changed logging.info to the "correct" kedro logger were in:

  • _register_hooks_setuptools
  • Session.run
  • DataCatalog.list

These are all ok because they're only ever called inside a kedro project, when we do want the info.log file.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@antonymilne antonymilne marked this pull request as ready for review November 22, 2021 11:23
@antonymilne antonymilne requested a review from idanov as a code owner November 22, 2021 11:23
@antonymilne antonymilne marked this pull request as draft November 22, 2021 11:23
@antonymilne antonymilne marked this pull request as ready for review November 22, 2021 11:54
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

Good catch! Thank you for fixing it so quickly! 👏

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

👍

@lorenabalan lorenabalan merged commit 7998c3e into master Nov 23, 2021
@lorenabalan lorenabalan deleted the bugfix/remove-annoying-log-error branch November 23, 2021 12:08
Galileo-Galilei pushed a commit to Galileo-Galilei/kedro that referenced this pull request Feb 19, 2022
lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022
@antonymilne antonymilne mentioned this pull request Apr 21, 2022
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants