Dnf automatic email on error #2005
Conversation
|
Hello @derickdiaz! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-10-27 17:00:30 UTC |
|
Please, could you squash the commits? You can keep adding yourself to authors in a separate one. |
derickdiaz
force-pushed
the
dnf-automatic-email-on-error
branch
from
803d258
to
e48f562
Compare
|
@m-blaha I squashed all the commits. Unfortunately, adding my name to the author's file was not a separate commit. |
derickdiaz
force-pushed
the
dnf-automatic-email-on-error
branch
from
e48f562
to
14882a9
Compare
No worries, it's much better now. Thank you. |
There was a problem hiding this comment.
The changes themselves look good to me. But I am not sure if we want this as the default behavior, meaning sending error reports to all clients, including the existing ones, which will experience a change in behavior. Perhaps we can provide a configuration option, such as send_error_messages? @j-mracek, please what are your thoughts on this?
dnf/automatic/main.py
Outdated
| @@ -367,9 +368,13 @@ def main(args): | |||
| exit_code = os.waitstatus_to_exitcode(os.system(conf.commands.reboot_command)) | |||
| if exit_code != 0: | |||
| logger.error('Error: reboot command returned nonzero exit code: %d', exit_code) | |||
| emitters.notify_error('Error: reboot command returned nonzero exit code: %d', exit_code) | |||
| emitters.commit() | |||
There was a problem hiding this comment.
I think that result of calling emitters.commit() multiple times (here and on line#364) will be sending multiple emails. Is that intended?
There was a problem hiding this comment.
It is possible that the emitter would commit twice in the case updated were applied successfully and the reboot command does not work. I could put an else statement and move the emitters.commit() in line 364 within the else statement.
Another problem I see is if a reboot error occurs, the emitter would only show that a reboot was unsuccessful instead of including the updates that were applied.
There was a problem hiding this comment.
I'm okay with that. Unless I missed something, it means that if there's an issue with rebooting, it will send an extra error email. This happens only if the user has enabled send_error_messages, which is not the default setting.
| @@ -72,14 +78,20 @@ def notify_downloaded(self): | |||
| assert self._available_msg | |||
| self._downloaded = True | |||
|
|
|||
| def notify_error(self, msg): | |||
| self._error = True | |||
| self._error_msg = msg | |||
There was a problem hiding this comment.
Is it possible to have error == True, but no message?
There was a problem hiding this comment.
Not necessarily. It's possible to leave an empty string as the msg variable; however, when the _prepare_msg function is called, "An error has occurred on <system_name>" will be the first thing on the error message before appending the body. If needed, I can add a default value to self._error_msg but it would be a generic error message.
Yes, such a change in stable might be a problem. I think it could be covered by configuration option and configuration might be modified for particular downstream, or the patch might be reverted in stable downstreams and only delivered to rawhide. According to your comment it looks like that the configuration should be used only temporary. But what's you @jan-kolarik preferences as a release master? |
It makes more sense to me not to have it enabled by default and allow users to enable this feature from the configuration. The reason is that DNF Automatic has been available for several years, and as far as I know, there haven't been any requests for such a feature. I consider it more of a 'nice-to-have' option. Additionally, existing users would not be affected. |
Added option 'send_error_messages' Fixed Option String List Changed option to Boolean
derickdiaz
force-pushed
the
dnf-automatic-email-on-error
branch
from
a89c07a
to
7c3cf97
Compare
|
LGTM, just please fix that typo in the man pages. We can merge it then if no one is against it. 🙂 |
The changes implemented in this PR are for: #1918
The changes allows the emitters to invoke when a DNF error occurs. I tested this out by setting the email_via = command_email and command_format = "echo {subject} {body} > /tmp/dnf_error" in the /etc/dnf/automatic.conf file. When I removed by GPG keys from the enabled repositories and run the def-automatic command it created the error file with the error description.