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

automatic: Return an error when transaction fails (RhBug:2170093) #1923

Merged
merged 1 commit into from
May 17, 2023

Conversation

jan-kolarik
Copy link
Member

@jan-kolarik jan-kolarik commented Apr 20, 2023

In case of no global error occurred within the transaction, we still need to check state of individual transaction items for any failure.

This is matching the logic in BaseCli.do_transaction method, where the error is emitted after printing the transaction summary.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2170093.
Tests: rpm-software-management/ci-dnf-stack#1251.

In case of no global error occurred within the transaction, we still need to check state of individual transaction items for any failure.

This is matching the logic in `BaseCli.do_transaction` method, where the error is emitted after printing the transaction summary.

= changelog =
msg: automatic: Return an error when transaction fails
type: bugfix
resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2170093
# we need to check state of individual transaction items.
for tsi in trans:
if tsi.state == libdnf.transaction.TransactionItemState_ERROR:
raise dnf.exceptions.Error(_('Transaction failed'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a mistake but an alternative. What about to modify the last live and directly return 1?

        logger.error(_('Error: Transaction failed'))
        return 1

Copy link
Member

Choose a reason for hiding this comment

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

I was also considering this, but then I figured there was no real benefit from the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also the exact same code block is used here from which it was copied.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do some better and deeper refactoring, but I am not sure it's worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only benefit is for translators.

@j-mracek
Copy link
Contributor

LGTM

@j-mracek j-mracek merged commit 9e4f0eb into master May 17, 2023
@j-mracek j-mracek deleted the jkolarik/dnf-automatic-error-report branch May 17, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants