[RHELC-1375] Port RestorableFile to BackupController #1045

r0x0d · 2024-01-24T16:16:50Z

First part of the port for the remaining items for the BackupController.

Changes included in this work are:

Remove the old RestorableFile class in order to use the NewRestorableFile
Rename the NewRestorableFile back to RestorableFile
Fix a bug where test pollution was happening in the BackupController global object

Jira Issues: RHELC-1375

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

convert2rhel/backup.py

r0x0d · 2024-01-24T16:18:18Z

TODO

Test the changes in a VM
If [RHELC-1361] Fix file clash in NewRestorableFile #1050 gets merged first, port the changes to this PR.

codecov · 2024-01-24T16:18:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7b1ae12) 94.33% compared to head (eb1b5cf) 94.42%.
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
+ Coverage   94.33%   94.42%   +0.09%     
==========================================
  Files          47       48       +1     
  Lines        4552     4523      -29     
  Branches      811      805       -6     
==========================================
- Hits         4294     4271      -23     
+ Misses        182      178       -4     
+ Partials       76       74       -2

Flag	Coverage Δ
centos-linux-7	`89.58% <100.00%> (+0.06%)`	⬆️
centos-linux-8	`90.58% <100.00%> (+0.07%)`	⬆️
centos-linux-9	`90.64% <100.00%> (+0.07%)`	⬆️

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.

has-bot · 2024-01-24T16:19:18Z

/packit test --labels tier0

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

Log | Bot Usage

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

First part of the port for the remaining items for the BackupController. Changes included in this work are: 1. Remove the old RestorableFile class in order to use the NewRestorableFile 2. Rename the NewRestorableFile back to RestorableFile 3. Fix a bug where test pollution was happening in the BackupController global object Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

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

convert2rhel/backup/files.py

+
+from convert2rhel import exceptions
+from convert2rhel.backup import RestorableChange
+from convert2rhel.utils import BACKUP_DIR


convert2rhel/redhatrelease.py

@@ -21,7 +21,8 @@
 import os
 import re

-from convert2rhel import backup, pkgmanager, utils
+from convert2rhel import pkgmanager, utils


convert2rhel/redhatrelease.py

@@ -21,7 +21,8 @@
 import os
 import re

-from convert2rhel import backup, pkgmanager, utils
+from convert2rhel import pkgmanager, utils
+from convert2rhel.backup.files import RestorableFile


convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

Refactoring the way we organize the files backup related code. Currently, we just throw everything inside the backup.py module and call the classes/functions from there. With this commit, we refactor have it's own module inside the backup folder. The idea is to bring all of the others backup codes to it's own module as well, and leave the backup/__init__.py to be a base module that will have only base code for the other modules. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d · 2024-01-24T19:45:52Z

In this PR, I'm trying to refactor the way we organize the backup code related to files and the other modules.

The idea is that in the next PRs we take the code from the backup/init.py and split them into their own modules and keep organizing the backup there.

If people think that the complexity increases, we can rollback to the original and move from there... But I strongly think that it will help to organize the code and give the idea that the backup is an API to be used across the rest of the project.

Venefilyn

Didn't look over the unit tests yet

convert2rhel/backup/files.py

Venefilyn · 2024-01-25T15:18:31Z

convert2rhel/backup/files.py

+                # Do not call 'critical' which would halt the program. We are in
+                # a rollback phase now and we want to rollback as much as possible.
+                loggerinst.warning("Error(%s): %s" % (err.errno, err.strerror))
+                return


Same here, do we want to use critical_no_exit()?

Maybe it would help if we made the choices critical and critical_exit? The one that exits is the one with the side effect (and a significant side effect at that), which argues that it should be the one with the more detailed name. It would also make the code more clear to someone just browsing it.

Agreed @jochapma, we made it this way for making it easier to migrate later. Easier to add critical_no_exit() and call it than to refactor the whole codebase. Technical debt in the making but something we had to do last year

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

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

Venefilyn

LGTM

convert2rhel/backup/files.py

Venefilyn · 2024-01-30T13:41:06Z

/packit test --labels tier0

convert2rhel/backup/files.py

hosekadam · 2024-01-30T15:21:37Z

I like the way how you split the backup and organized it into the modules. I agree that it help with organizing it, having it in one file was a bit messy - especially after adding more restorable things.

This commit prevents the RestorableFile being enabled if the target filepath is not a file. It also removes duplicated tests cases where the test was the same, but using the backup controller instead. It is not required to use backup controller instance for those type of tests, since we want to test the RestorableFile class itself, not it's integration with the controller. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d · 2024-01-30T18:43:58Z

/packit test --labels sanity

has-bot · 2024-01-30T18:44:28Z

/packit test --labels sanity

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

Log | Bot Usage

r0x0d · 2024-01-30T19:42:45Z

/packit retest-failed

r0x0d · 2024-01-31T12:45:39Z

/packit test --labels sanity

jochapma

Looks good to me.

r0x0d · 2024-02-01T13:06:10Z

The test failures doesn't seem related to the changes in the PR. Merging this now to unblock us with the next tasks.

r0x0d · 2024-02-01T13:06:32Z

/packit test

r0x0d · 2024-02-02T12:35:31Z

Tests failure seems to be unrelated. Merging this righ now to unblock other work.

r0x0d commented Jan 24, 2024

View reviewed changes

convert2rhel/backup.py Outdated Show resolved Hide resolved

r0x0d added kind/feature New feature or request tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels Jan 24, 2024

github-advanced-security bot found potential problems Jan 24, 2024

View reviewed changes

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py Fixed Show fixed Hide fixed

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py Fixed Show fixed Hide fixed

r0x0d force-pushed the port-restorable-file-backup branch from f582e45 to 0a57952 Compare January 24, 2024 17:32

r0x0d added 2 commits January 24, 2024 14:44

Rename test reference

f0d8add

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

Fix CodeQL warnings

09d63cc

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

r0x0d force-pushed the port-restorable-file-backup branch from 8583bd1 to 3e70eba Compare January 24, 2024 19:15

github-advanced-security bot found potential problems Jan 24, 2024

View reviewed changes

r0x0d force-pushed the port-restorable-file-backup branch from 3e70eba to bfba68b Compare January 24, 2024 19:39

Venefilyn requested changes Jan 25, 2024

View reviewed changes

r0x0d changed the title ~~[RHELC-1153] Port RestorableFile to BackupController~~ [RHELC-1375] Port RestorableFile to BackupController Jan 25, 2024

Apply review suggestions

a7175da

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

r0x0d requested a review from Venefilyn January 26, 2024 14:11

Venefilyn approved these changes Jan 30, 2024

View reviewed changes

convert2rhel/backup/files.py Outdated Show resolved Hide resolved

hosekadam reviewed Jan 30, 2024

View reviewed changes

convert2rhel/backup/files.py Outdated Show resolved Hide resolved

r0x0d added tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. and removed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels Jan 30, 2024

pr-watson approved these changes Jan 30, 2024

View reviewed changes

hosekadam approved these changes Jan 31, 2024

View reviewed changes

jochapma approved these changes Jan 31, 2024

View reviewed changes

r0x0d merged commit ade78f9 into oamg:main Feb 2, 2024
31 of 63 checks passed

r0x0d deleted the port-restorable-file-backup branch February 2, 2024 12:35

Venefilyn mentioned this pull request Feb 22, 2024

Release v1.7.0 #1111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1375] Port RestorableFile to BackupController #1045

[RHELC-1375] Port RestorableFile to BackupController #1045

r0x0d commented Jan 24, 2024 •

edited

Loading

r0x0d commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading

has-bot commented Jan 24, 2024

r0x0d commented Jan 24, 2024

Venefilyn left a comment

Venefilyn Jan 25, 2024 •

edited

Loading

jochapma Jan 26, 2024

Venefilyn Jan 30, 2024

Venefilyn left a comment

Venefilyn commented Jan 30, 2024

hosekadam commented Jan 30, 2024

r0x0d commented Jan 30, 2024

has-bot commented Jan 30, 2024

r0x0d commented Jan 30, 2024

r0x0d commented Jan 31, 2024

jochapma left a comment

r0x0d commented Feb 1, 2024

r0x0d commented Feb 1, 2024

r0x0d commented Feb 2, 2024

[RHELC-1375] Port RestorableFile to BackupController #1045

[RHELC-1375] Port RestorableFile to BackupController #1045

Conversation

r0x0d commented Jan 24, 2024 • edited Loading

r0x0d commented Jan 24, 2024 • edited Loading

TODO

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

has-bot commented Jan 24, 2024

r0x0d commented Jan 24, 2024

Venefilyn left a comment

Choose a reason for hiding this comment

Venefilyn Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

jochapma Jan 26, 2024

Choose a reason for hiding this comment

Venefilyn Jan 30, 2024

Choose a reason for hiding this comment

Venefilyn left a comment

Choose a reason for hiding this comment

Venefilyn commented Jan 30, 2024

hosekadam commented Jan 30, 2024

r0x0d commented Jan 30, 2024

has-bot commented Jan 30, 2024

r0x0d commented Jan 30, 2024

r0x0d commented Jan 31, 2024

jochapma left a comment

Choose a reason for hiding this comment

r0x0d commented Feb 1, 2024

r0x0d commented Feb 1, 2024

r0x0d commented Feb 2, 2024

r0x0d commented Jan 24, 2024 •

edited

Loading

r0x0d commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading

Venefilyn Jan 25, 2024 •

edited

Loading