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

[RHELC-778] Add a progress indicator for the main package replacement #709

Merged
merged 12 commits into from
Feb 9, 2023

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Jan 18, 2023

Add progress indicators for DNF/YUM

This commit introduces the callbacks classes for the progress indicators
that we will be showing for the user during a few different phases in
our code.

The callbacks will called during these phases:

  • For DNF: Dependency Solver, Package Download and Transaction Progress
  • For YUM: Package Download and Transaction Progress

We don't have a dependency solver callback for yum, because the yum code
base does not provide any base class for us to override and iterate
over, thus, we would require to copy and paste their own dependency
resolver callback and deal adjust the code with the things we want and
the ones we don't care. Since those classes are highly tied to the
base usage, it is possible that we could mess with something and lead
to obscure errors, thus, we are not using any dependency solver callback
for YUM.

Signed-off-by: Rodolfo Olivieri rolivier@redhat.com

Jira Issue: RHELC-778

Checklist

  • PR meets acceptance criteria specified in the Jira issue
  • 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
  • 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

@r0x0d r0x0d self-assigned this Jan 18, 2023
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Base: 92.45% // Head: 92.69% // Increases project coverage by +0.24% 🎉

Coverage data is based on head (ea360c0) compared to base (0b261dc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
+ Coverage   92.45%   92.69%   +0.24%     
==========================================
  Files          22       24       +2     
  Lines        3166     3270     +104     
  Branches      557      576      +19     
==========================================
+ Hits         2927     3031     +104     
  Misses        172      172              
  Partials       67       67              
Flag Coverage Δ
centos-linux-7 87.95% <50.44%> (-1.25%) ⬇️
centos-linux-8 89.08% <76.99%> (-0.31%) ⬇️

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

Impacted Files Coverage Δ
convert2rhel/pkgmanager/__init__.py 94.73% <100.00%> (+1.40%) ⬆️
convert2rhel/pkgmanager/handlers/dnf/__init__.py 100.00% <100.00%> (ø)
convert2rhel/pkgmanager/handlers/dnf/callback.py 100.00% <100.00%> (ø)
convert2rhel/pkgmanager/handlers/yum/__init__.py 97.91% <100.00%> (ø)
convert2rhel/pkgmanager/handlers/yum/callback.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@r0x0d
Copy link
Member Author

r0x0d commented Jan 23, 2023

/packit test

@danmyway
Copy link
Member

On a call with @r0x0d and @kokesak, we have agreed that verification for this feature would be more or less sanity check. The nature of the implemented changes is cosmetic, therefore should not break anything inside the conversion. The current test suite is sufficient to catch any failures.
We have also agreed on, that we will implement a sanity check in the future, which will match a list of strings in the log file after conversion. This feature output should be also implemented in said sanity check.

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.

Not tested it in VM

convert2rhel/pkgmanager/__init__.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/__init__.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/yum/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/__init__.py Outdated Show resolved Hide resolved
@Venefilyn Venefilyn added the kind/feature New feature or request label Jan 24, 2023
@r0x0d r0x0d requested a review from Venefilyn January 24, 2023 16:39
@r0x0d r0x0d force-pushed the add-progress-indicators branch 4 times, most recently from 4616ab4 to 7724712 Compare January 26, 2023 12:18
MANIFEST.in Outdated Show resolved Hide resolved
@r0x0d r0x0d requested a review from abadger January 27, 2023 14:34
@@ -15,20 +15,25 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <https://www.gnu.org/licenses/>.


Copy link
Member Author

Choose a reason for hiding this comment

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

Another future note for me: This might get merge conflicts if #713 is merged first.

abadger
abadger previously approved these changes Feb 8, 2023
Copy link
Member

@abadger abadger left a comment

Choose a reason for hiding this comment

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

One code change (that is very minor) and some grammar and spelling changes which aren't needed. This looks good to me. Note that I have not tested the change in a VM. Approving if you have tested.

convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/yum/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/yum/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
convert2rhel/pkgmanager/handlers/dnf/callback.py Outdated Show resolved Hide resolved
Venefilyn
Venefilyn previously approved these changes Feb 8, 2023
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.

Overall seems good to me not tested it in VM. If you can demo this next week on the demo session on Tuesday that would be great @r0x0d

pr-watson
pr-watson previously approved these changes Feb 8, 2023
Copy link
Contributor

@pr-watson pr-watson left a comment

Choose a reason for hiding this comment

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

These changes look good to me, approved

abadger
abadger previously approved these changes Feb 8, 2023
@r0x0d
Copy link
Member Author

r0x0d commented Feb 8, 2023

Overall seems good to me not tested it in VM. If you can demo this next week on the demo session on Tuesday that would be great @r0x0d

Nice idea! I will add that to the document

@r0x0d
Copy link
Member Author

r0x0d commented Feb 8, 2023

Woops, file conflicts... I will do ahead and fix them.

This commit introduces the callbacks classes for the progress indicators
that we will be showing for the user during a few different phases in
our code.

The callbacks will called during these phases:

* For DNF: Dependency Solver, Package Download and Transaction Progress
* For YUM: Package Download and Transaction Progress

We don't have a dependency solver callback for yum, because the yum code
base does not provide any base class for us to override and iterate
over, thus, we would require to copy and paste their own dependency
resolver callback and deal adjust the code with the things we want and
the ones we don't care. Since those classes are highly tied to the
`base` usage, it is possible that we could mess with something and lead
to obscure errors, thus, we are not using any dependency solver callback
for YUM.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
r0x0d and others added 10 commits February 8, 2023 14:34
We are refactoring the DNF and YUM modules to be in a separate folder,
as, we are also placing the callback classes in it's own module, mostly
because we are placing as well the licenses of the modules that we are
using and adaptating from.

Since we had to separate the callback classes to a new module and not
place them in the previous `dnf.py` and `yum.py`, we went ahead and
moved also the `dnf.py` and `yum.py` to be an `__init__.py`, but they
are placed under their respective folders.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
This commit introduces the unit_tests regarding the progress indicators
that we added for both DNF and YUM.

It also follows the refactor that was done in
`convert2rhel/pkgmanager/handlers/*` to better accommodate the changes.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Adding more tests to cover 100% coverage for our dnf callback classes.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Co-authored-by: E Gustavsson <eric@spytec.se>
Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Since this work will introduce new folders and files, we figured out it
was best to automate the packages discovery when building the project,
rather than specifing manually every new folder to include.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Added two more functions to the callback classes to handle the cases
where a scriptlet will fail, or, the transaction will fail, so can we
output a log message to the user and save it to our log file.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
Co-authored-by: Preston Watson <104600742+pr-watson@users.noreply.github.com>
@r0x0d
Copy link
Member Author

r0x0d commented Feb 8, 2023

Sorry to who approved the PR.. It will need another approval :D

@r0x0d
Copy link
Member Author

r0x0d commented Feb 9, 2023

Pipeline is all green! Merging this as I already had approval before of the rebase.

@r0x0d r0x0d merged commit 8343dbb into oamg:main Feb 9, 2023
@r0x0d r0x0d deleted the add-progress-indicators branch February 9, 2023 12:32
@Venefilyn Venefilyn mentioned this pull request Feb 20, 2023
@Venefilyn Venefilyn changed the title [RHELC-778] Add progress indicators for YUM and DNF modules [RHELC-778] Add a progress indicator for the main package replacement Feb 22, 2023
Venefilyn added a commit that referenced this pull request Jun 19, 2023
* Add progress indicators for DNF/YUM

This commit introduces the callbacks classes for the progress indicators
that we will be showing for the user during a few different phases in
our code.

The callbacks will called during these phases:

* For DNF: Dependency Solver, Package Download and Transaction Progress
* For YUM: Package Download and Transaction Progress

We don't have a dependency solver callback for yum, because the yum code
base does not provide any base class for us to override and iterate
over, thus, we would require to copy and paste their own dependency
resolver callback and deal adjust the code with the things we want and
the ones we don't care. Since those classes are highly tied to the
`base` usage, it is possible that we could mess with something and lead
to obscure errors, thus, we are not using any dependency solver callback
for YUM.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Refactor the handlers module to have it's own folder

We are refactoring the DNF and YUM modules to be in a separate folder,
as, we are also placing the callback classes in it's own module, mostly
because we are placing as well the licenses of the modules that we are
using and adaptating from.

Since we had to separate the callback classes to a new module and not
place them in the previous `dnf.py` and `yum.py`, we went ahead and
moved also the `dnf.py` and `yum.py` to be an `__init__.py`, but they
are placed under their respective folders.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Add unit_tests for the progress indicators

This commit introduces the unit_tests regarding the progress indicators
that we added for both DNF and YUM.

It also follows the refactor that was done in
`convert2rhel/pkgmanager/handlers/*` to better accommodate the changes.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Fix CodeQL error

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Add more coverage to the dnf callback classes

Adding more tests to cover 100% coverage for our dnf callback classes.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Apply suggestions from code review

Co-authored-by: E Gustavsson <eric@spytec.se>

* Fix comments from reviews

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Update package discovery in setup.py

Since this work will introduce new folders and files, we figured out it
was best to automate the packages discovery when building the project,
rather than specifing manually every new folder to include.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Remove unused property in dnf callback

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Fix comments from last review and added more logs

Added two more functions to the callback classes to handle the cases
where a scriptlet will fail, or, the transaction will fail, so can we
output a log message to the user and save it to our log file.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Apply suggestions from code review

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>

* Apply suggestions from code review

Co-authored-by: Preston Watson <104600742+pr-watson@users.noreply.github.com>

---------

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Co-authored-by: E Gustavsson <eric@spytec.se>
Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
Co-authored-by: Preston Watson <104600742+pr-watson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants