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

Reorganize the error handler #3056

Merged
merged 14 commits into from Jan 11, 2021

Conversation

poncovka
Copy link
Contributor

We are not able to use the error handler in the DBus modules, so move
it from the low-level code to the top level if possible, so we don't have to
deal with it during the Anaconda modularization.

Remove error handlers for deprecated or invalid use cases and clean up
the code.

@poncovka poncovka added master Please, use the `f39` label instead. manual testing required This issue can't be merged without manual testing labels Dec 14, 2020
@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

⚠️ Please note that our current plans include removal of these comments in the near future (at least 2 weeks after including this disclaimer), if you have serious concerns regarding their removal or would like to continue receiving them please reach out to us. ⚠️

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/rhinstaller-anaconda-3056
  • And now you can install the packages.

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

@VladimirSlavik
Copy link
Contributor

Code looks far better, but won't our users miss all the wonderful detail in error popups?

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Great cleanup, thanks a lot for this!

I have a few question below which I would like to have answered before merge.

pyanaconda/core/users.py Show resolved Hide resolved
pyanaconda/payload/dnf/payload.py Show resolved Hide resolved
pyanaconda/errors.py Show resolved Hide resolved
pyanaconda/errors.py Show resolved Hide resolved
pyanaconda/payload/live/payload_liveos.py Show resolved Hide resolved
pyanaconda/payload/dnf/utils.py Show resolved Hide resolved
pyanaconda/errors.py Outdated Show resolved Hide resolved
Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Looks good to me - nice cleanup & removal of effectively dead code in hiding! :)

pyanaconda/errors.py Show resolved Hide resolved
pyanaconda/payload/rpmostreepayload.py Show resolved Hide resolved
pyanaconda/errors.py Outdated Show resolved Hide resolved
@poncovka poncovka marked this pull request as draft January 5, 2021 18:18
@poncovka poncovka force-pushed the master-payload_error_handler branch 2 times, most recently from d9970b6 to d88f148 Compare January 6, 2021 10:51
@poncovka
Copy link
Contributor Author

poncovka commented Jan 6, 2021

/kickstart-test --testtype coverage

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me, except that I wasn't able to spot the removed IPMI calls replacement.

We shouldn't handle errors in the code that is run by the installation tasks.
If the password cannot be encrypted with the specified algorithm, it is a bug.
We should fail to set up the source and report that to the user instead.

The mountImage function is called only from the _find_and_mount_iso method with
a normalized path of an ISO file. Therefore, we can remove the mountImage
function and use the mount function from the payload utils instead.
@poncovka poncovka force-pushed the master-payload_error_handler branch from d88f148 to a1de0e3 Compare January 8, 2021 11:19
We should fail to set up the source and report that to the user instead.
We are able to handle unknown groups. This is always a bug.
Move the mapping into an attribute and rename some of the handlers.
If there is no callback, then there is nothing to handle.
We will always raise the exception.
Let the installation task handle the exception raised in the code.
Any PayloadError exception is fatal anyway.
Rename the method to _handle_marking_error.
There is no InstallMoreStreamsException class in dnf.exceptions
and the NoStreamSpecifiedException class is not used by DNF.
Show a dialog window that informs a user why the non-package payload couldn't
be set up. There is no UI support that could handle the error state, so we need
to handle or raise the error directly. Make sure that payloads raise the right
exception from the setup method.
Log the IPMI events, so we can debug them when IPMI is not supported.
Make sure that we always report an IPMI event before calling sys.exit.
Fix some of the exit codes since the installation cannot be successful
in this case.
Make sure that exit dialog windows report an IPMI event before calling sys.exit.
TUI already works this way, so we just need to fix GUI to work the same way.
@poncovka poncovka force-pushed the master-payload_error_handler branch from a1de0e3 to b6d9133 Compare January 8, 2021 11:41
@poncovka
Copy link
Contributor Author

poncovka commented Jan 8, 2021

/kickstart-test --testtype coverage

@poncovka poncovka marked this pull request as ready for review January 8, 2021 11:55
@poncovka poncovka removed the manual testing required This issue can't be merged without manual testing label Jan 8, 2021
@poncovka
Copy link
Contributor Author

poncovka commented Jan 8, 2021

All issues should be resolved and the code is tested.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Great work! Thanks a lot for this!

@poncovka poncovka merged commit 557d680 into rhinstaller:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master Please, use the `f39` label instead.
4 participants