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

Don't turn off stdout/stderr logging longer than necessary (RhBug:1843280) #1635

Closed
wants to merge 1 commit into from

Conversation

pkratoch
Copy link
Contributor

When _setup failed (for example due to missing permissions to create the
log file), the error was not printed to stderr because of turning off
the sdtout/stderr handlers temporarily.

However, the stdout/stderr handlers need to be turned off only when
creating the logging marker.

https://bugzilla.redhat.com/show_bug.cgi?id=1843280

@pep8speaks
Copy link

pep8speaks commented Jun 29, 2020

Hello @pkratoch! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-01 13:37:10 UTC

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/rpm-software-management-dnf-1635
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 2f1ad36) made this pull request unmergeable. Please resolve the merge conflicts.

@pkratoch pkratoch force-pushed the 1843280 branch 2 times, most recently from 846f3db to 5e1aba3 Compare July 1, 2020 11:25
@pkratoch
Copy link
Contributor Author

pkratoch commented Jul 1, 2020

I brought back the verbose_level and error_level arguments, because the log handler has no method such as get_level. It does have a level attribute, but it's not documented, so I'd rather not use it.

I also wandered if it couldn't be resolved more elegantly, but I didn't find any other way to disable individual handlers (just the whole logger) and emitting a record straight into the one handler seems too hacky (though it would just mean creating a LogRecord and calling handle, so maybe it's not much worse than current code).

@@ -209,10 +201,6 @@ def _setup(self, verbose_level, error_level, logfile_level, logdir, log_size, lo
logger_rpm.addHandler(self.stdout_handler)
logger_rpm.addHandler(self.stderr_handler)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just paint both log file marks at this point, I don't think it matters much that they will happen a bit later. Then you don't need to forward the levels to _setup_file_loggers() and you can also turn them off at this place once, paint both marks and turn them back on.

I would also set the level to WARNING, not SUPERCRITICAL, so that any errors won't actually be filtered out, but the paint mark at INFO will.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! I modified the PR according to your suggestions.

…3280)

When _setup failed (for example due to missing permissions to create the
log file), the error was not printed to stderr because of turning off
the sdtout/stderr handlers temporarily.

However, the stdout/stderr handlers need to be turned off only when
creating the logging marker.

https://bugzilla.redhat.com/show_bug.cgi?id=1843280
@lukash
Copy link
Contributor

lukash commented Jul 1, 2020

Looks a bit better now 🙂

@m-blaha I hope you don't mind me merging this...

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 41035e8 has been approved by lukash

@rh-atomic-bot
Copy link

⌛ Testing commit 41035e8 with merge fb1ea32...

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: lukash
Pushing fb1ea32 to master...

@pkratoch pkratoch deleted the 1843280 branch July 2, 2020 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants