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

Fix history rollback command does not meet the expected result #2042

Closed

Conversation

chenhaixing123
Copy link
Contributor

@chenhaixing123 chenhaixing123 commented Jan 30, 2024

= changelog =
msg: Fix history rollback command does not meet the expected result
type: bugfix
resolves: #2031
related: #1689

"Obsolete": "Obsoleted",
"Reason Change": "Reason Change",
"Obsolete": None,
"Reason Change": None,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think this is right. It breaks the workflow when rolling back group and environment upgrades while reviewing test results. While the actual action appears to be executed, the group and environment reasons are not recorded in the history database. We need to rather investigate the root cause of the reproducer outlined in the issue.

@@ -536,13 +536,18 @@ def run(self):

for pkg_data in self._rpms:
try:
action = pkg_data["action"]
if action is None:
continue
Copy link
Member

Choose a reason for hiding this comment

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

This is also not correct, we would miss logging the unexpected state transitions.

@jan-kolarik
Copy link
Member

@chenhaixing123 Thanks for submitting this one, it seems that it pointed me to the exact point of the problem here. I've prepared a follow-up PR for that. Let's wait for the tests and we can merge that one instead, what do you think?

@chenhaixing123
Copy link
Contributor Author

Thank you for reviewing the PR.
I'm still a little confused. If the merge action is to reinstall, do we execute the rollback command to reinstall or do nothing?

@jan-kolarik
Copy link
Member

Thank you for reviewing the PR. I'm still a little confused. If the merge action is to reinstall, do we execute the rollback command to reinstall or do nothing?

I'll probably need to rework the new PR. The root cause is probably somewhere else, more likely in the library code. I'll send the update with the findings in the PR. For now, it seems to me "more correct" to do nothing in such cases.

@jan-kolarik
Copy link
Member

I've prepared another PR to fix the issue already in the library code:

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.

Unexpected results in 'dnf history rollback'
2 participants