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.

Sign up for GitHub

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

Jump to bottom

[RHELC-1270] Update package handling check to properly report removed packages #999

Merged
merged 1 commit into from
Dec 4, 2023
Merged

[RHELC-1270] Update package handling check to properly report removed packages #999

merged 1 commit into from
Dec 4, 2023

Conversation

pr-watson
Copy link
Contributor

@pr-watson pr-watson commented Nov 30, 2023 •
edited
Loading

With the current setup of the conditionals in handle_packages, there are cases where a message explaining that packages were removed will be made regardless if there are any packages removed or not. This PR changes the conditionals to only display the packages removed message when there are packages contained inside the pkgs_removed variable.

Jira Issues: RHELC-1270

image

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • GitHub label has been added to help with Release notes
  • PR title explains the change from the user's point of view
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending if relevant

@codecov Codecov
Copy link

codecov bot commented Nov 30, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1245713) 94.22% compared to head (43e806c) 94.18%.
Report is 10 commits behind head on main.

Files Patch % Lines
...t2rhel/actions/pre_ponr_changes/handle_packages.py 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #999      +/-   ##
==========================================
- Coverage   94.22%   94.18%   -0.05%     
==========================================
  Files          47       47              
  Lines        4378     4382       +4     
  Branches      775      777       +2     
==========================================
+ Hits         4125     4127       +2     
  Misses        177      177              
- Partials       76       78       +2
Flag Coverage Δ
centos-linux-7 89.15% <50.00%> (-0.04%) ⬇️
centos-linux-8 90.21% <50.00%> (-0.04%) ⬇️
centos-linux-9 90.27% <50.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-watson pr-watson added kind/feature New feature or request tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. labels Nov 30, 2023
@has-bot
Copy link
Member

has-bot commented Nov 30, 2023

/packit test --labels sanity

@oamg/conversions-qe please review results and provide ack.

Venefilyn
Venefilyn requested changes Nov 30, 2023
View reviewed changes
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

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

If there is a Jira issue about fixing this wording, please assign the PR to that. Otherwise create one so it's documented that we address this somewhere

convert2rhel/actions/pre_ponr_changes/handle_packages.py
@@ -207,13 +207,13 @@ def run(self):
description="Repository file packages which could not be removed",
diagnosis=message,
)
else:
if pkgs_removed:
Copy link
Member

Choose a reason for hiding this comment

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

Keep in mind that pkgs_removed wasn't declared outside of the try-catch, so this would fail if that ever changes. I would initialize pkgs_removed before it gets sorted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But with the current state of the code wouldn't it be fine?
This line pkgs_removed = sorted(pkghandler.remove_pkgs_unless_from_redhat(pkgs_to_remove) or []) ensures that we either get the removed packages or an empty list.

Copy link
Member

Choose a reason for hiding this comment

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

It does! However, it is within a try-catch. So if we change the try catch in the future to not exit, we'd run into the issue where pkgs_removed might be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay that makes sense, let me initialize pkgs_removed

@pr-watson pr-watson changed the title Update package handling check to properly report removed packages [RHELC-1270] Update package handling check to properly report removed packages Dec 1, 2023
Venefilyn
Venefilyn approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

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

LGTM

@pr-watson
Copy link
Contributor Author

/packit test --labels sanity

@pr-watson pr-watson merged commit dfc2e2a into oamg:main Dec 4, 2023
15 of 49 checks passed
@pr-watson pr-watson mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Venefilyn Venefilyn Venefilyn approved these changes

Assignees
No one assigned
Labels
kind/feature New feature or request tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pr-watson @has-bot @Venefilyn