-
Notifications
You must be signed in to change notification settings - Fork 150
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
pes_events_scanner: fix processing of RpmTransactionTasks #1296
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
Note that first time contributors cannot run tests automatically - they need to be started by a reviewer. It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
See other labels for particular jobs defined in the Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
/packit copr-build |
@pirat89 Although this fixes the crash, I believe that transaction configuration is handled in a weird manner altogether. If user says |
@MichalHe you are right that this sounds weird. feel free to fix it properly in this PR - or close this one and open new one if you want. this has been really a draft to ilustrate what I found and initial possible fix of the problem. based on your finding, it seems there is something dirty still around. |
35e9a43
to
130a56b
Compare
@pirat89 I added a post-processing step that will add transaction configuration's fields to the scanner's output. |
130a56b
to
ddbe786
Compare
RpmTransactionTasks messages have higher priority than instructions based on PES data. Previously, if multiple such messages existed with duplicate instructions, this could lead to the crash of the actor - especially in case when an existing package has been asked to be removed several times. Ensure the occurance of each instruction is unique (list -> set). jira: RHEL-50076
1f0602f
to
2daae22
Compare
/packit build |
I have tested the patch on a setup where I introduced an actor that produces: self.produce(RpmTransactionTasks(to_remove=['tomcat', 'tomcat']))
self.produce(RpmTransactionTasks(to_remove=['tomcat'])) Using current upstream build, the upgrade crashed with the following error:
Using build based on this PR results with no errors and the upgrade continues normally, without raising the above error. Hence, I believe this PR is ready for review. |
2daae22
to
a81fa1b
Compare
repos/system_upgrade/common/actors/peseventsscanner/libraries/pes_events_scanner.py
Outdated
Show resolved
Hide resolved
Previously, pes_events_scanner used transaction configuration to only modify the way it initializes event application. As a consequence, if a user specified to_remove=['pkg'], then the information would not make it to pes_events_scanner's output. Similar situation would arise with to_install/to_keep. This patch adds a post-processing to explicitly add transaction configuration to the result of applying PES events.
a81fa1b
to
4a10ecc
Compare
giving +1 for @MichalHe changes. considering it good to go, but as original author of the PR, waiting for someone else to review it and merge it - even when my changes are minor in this PR. |
The failing 9>10 tests are not related to this PR. Since two of us are OK with this PR, I am going to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks OK to me, hence we have agreement of 2 developers. e2e tests are passing, so lets merge this
RpmTransactionTasks messages have higher priority than instructions based on PES data. Previously, if multiple such messages existed with duplicate instructions, this could lead to the crash of the actor - especially in case when an existing package has been asked to be removed several times. Ensure the occurance of each instruction is unique (list -> set).
jira: RHEL-50076
Note: just written on knees without testing. Unit tests should be updated too.