[RHELC-1153] Port the restorable file to work with backup controller #935

hosekadam · 2023-09-26T14:09:41Z

This PR ports the restorable file to work with backup controller. No functionality changed, the old restorable file is still being used and waiting for complete port. This is mainly for #875 where this part of code is going to be used for now.

Jira Issues:

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

codecov · 2023-09-27T09:08:22Z

Codecov Report

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

Comparison is base (45cc026) 94.21% compared to head (16c6c0b) 94.21%.
Report is 8 commits behind head on main.

Files	Patch %	Lines
convert2rhel/backup.py	94.02%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #935      +/-   ##
==========================================
- Coverage   94.21%   94.21%   -0.01%     
==========================================
  Files          47       47              
  Lines        4377     4444      +67     
  Branches      775      787      +12     
==========================================
+ Hits         4124     4187      +63     
- Misses        177      180       +3     
- Partials       76       77       +1

Flag	Coverage Δ
centos-linux-7	`89.26% <94.02%> (+0.07%)`	⬆️
centos-linux-8	`90.30% <94.02%> (+0.05%)`	⬆️
centos-linux-9	`90.36% <94.02%> (+0.05%)`	⬆️

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.

jochapma

It seems risky to me to just return if the backup_filepath doesn't exist in restore() at line 383; should the info message at least be a warning?

jochapma

In restore(), maybe you should call the superclass restore method?

hosekadam · 2023-10-10T10:42:48Z

@jochapma about your questions:

"should the info message at least be a warning?"
It's this line of code (just for making sure we both are talking about the same): https://github.com/oamg/convert2rhel/pull/935/files#diff-8f4622020cde9a253bc82f69bf2b88910bcc5a84943db3c7d3a435becedfd4d5R381
I used the same part of code as we have now there: https://github.com/oamg/convert2rhel/blob/main/convert2rhel/backup.py#L369

As I'm thinking about it now, it might be better to warn the user during the enabling (backing up) the file, since during the rollback the user can't do anything with it and the warning is useless from my point of view. I'm not sure if exists case if this message would be printed, but I didn't want to get rid of this case.

And as I'm looking into the code https://github.com/oamg/convert2rhel/pull/935/files#diff-8f4622020cde9a253bc82f69bf2b88910bcc5a84943db3c7d3a435becedfd4d5R354 it's mostly fine I would say. If the path doesn't exists we inform the user. If there we reach some exception, the critical is printed.

"In restore(), maybe you should call the superclass restore method?"
I'm calling it there: https://github.com/oamg/convert2rhel/pull/935/files#diff-8f4622020cde9a253bc82f69bf2b88910bcc5a84943db3c7d3a435becedfd4d5R399 or you meant something else?

pr-watson

Code looks good to me! Approved

Venefilyn · 2023-10-31T14:36:51Z

convert2rhel/backup.py

+    def enable(self):
+        """Save current version of a file"""
+        # Prevent multiple backup
+        if self.enabled:
+            return
+
+        loggerinst.info("Backing up %s." % self.filepath)
+        if os.path.isfile(self.filepath):
+            try:
+                loggerinst.debug("Copying %s to %s." % (self.filepath, BACKUP_DIR))
+                shutil.copy2(self.filepath, BACKUP_DIR)
+
+            except (OSError, IOError) as err:
+                # IOError for py2 and OSError for py3
+                loggerinst.critical("Error(%s): %s" % (err.errno, err.strerror))
+        else:
+            loggerinst.info("Can't find %s.", self.filepath)
+
+        # Set the enabled value
+        super(NewRestorableFile, self).enable()
+
+    def restore(self, rollback=True):
+        """Restore a previously backed up file"""
+        # We do not have backup
+        if not self.enabled:
+            loggerinst.info("%s hasn't been backed up." % self.filepath)
+            return
+
+        backup_filepath = os.path.join(BACKUP_DIR, os.path.basename(self.filepath))
+        if rollback:
+            loggerinst.task("Rollback: Restore %s from backup" % self.filepath)
+        else:
+            loggerinst.info("Restoring %s from backup" % self.filepath)
+
+        if not os.path.isfile(backup_filepath):
+            loggerinst.info("%s hasn't been backed up." % self.filepath)
+            return
+
+        try:
+            shutil.copy2(backup_filepath, self.filepath)
+        except (OSError, IOError) as err:


Lots of paths not being tested here

@SpyTec I'm not sure if I understood. Do you mean not tested by unit tests or something else?

Yes that's right, L353 L381 L389 L393-394 L399

hosekadam · 2023-11-15T16:33:16Z

@SpyTec Added the unittests - hope I haven't missed anything. Changed roder of messages in restore method.

hosekadam · 2023-11-24T13:45:08Z

Changed message from comment #875 (comment)

Venefilyn · 2024-01-08T15:16:47Z

convert2rhel/backup.py

+            except (OSError, IOError) as err:
+                # IOError for py2 and OSError for py3
+                loggerinst.critical("Error(%s): %s" % (err.errno, err.strerror))


This doesn't seem to have unit tests covering it. Unless I'm mistaken the Error we currently catch is for restore() function

Venefilyn · 2024-01-08T15:18:30Z

convert2rhel/backup.py

+        if self.enabled:
+            return


We're testing the restore() when enabled is off but not enable()

hosekadam · 2024-01-11T15:57:19Z

@SpyTec I would say we can close this PR as it was part of #875 which is already merged and released

Venefilyn · 2024-01-11T16:07:22Z

@SpyTec I would say we can close this PR as it was part of #875 which is already merged and released

Gotcha, makes sense

hosekadam mentioned this pull request Sep 26, 2023

[RHELC-1153] Port the restorable file to work with backup controller #934

Closed

8 tasks

hosekadam force-pushed the new_restorable_file branch from 702e1c0 to 27ab6e2 Compare September 26, 2023 14:12

github-advanced-security bot found potential problems Sep 26, 2023

View reviewed changes

convert2rhel/backup.py Fixed Show resolved Hide resolved

hosekadam force-pushed the new_restorable_file branch from 27ab6e2 to be344e7 Compare September 27, 2023 09:06

hosekadam mentioned this pull request Sep 27, 2023

[RHELC-855, RHELC-1153] Recover file changes during a rollback #875

Merged

8 tasks

jochapma reviewed Sep 27, 2023

View reviewed changes

pr-watson approved these changes Oct 10, 2023

View reviewed changes

hosekadam force-pushed the new_restorable_file branch from be344e7 to 67db959 Compare October 11, 2023 12:05

Venefilyn reviewed Oct 31, 2023

View reviewed changes

hosekadam force-pushed the new_restorable_file branch from 67db959 to 042f62a Compare November 15, 2023 16:32

hosekadam force-pushed the new_restorable_file branch from 042f62a to 01fff71 Compare November 24, 2023 13:44

hosekadam force-pushed the new_restorable_file branch 2 times, most recently from 5b02587 to 93e47b5 Compare December 4, 2023 12:39

Port of restorable file to work with backupcontroller

16c6c0b

hosekadam force-pushed the new_restorable_file branch from 93e47b5 to 16c6c0b Compare December 5, 2023 14:57

jochapma approved these changes Dec 6, 2023

View reviewed changes

Venefilyn reviewed Jan 8, 2024

View reviewed changes

Venefilyn closed this Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1153] Port the restorable file to work with backup controller #935

[RHELC-1153] Port the restorable file to work with backup controller #935

hosekadam commented Sep 26, 2023

codecov bot commented Sep 27, 2023 •

edited

Loading

jochapma left a comment

jochapma left a comment

hosekadam commented Oct 10, 2023

pr-watson left a comment

Venefilyn Oct 31, 2023

hosekadam Nov 6, 2023

Venefilyn Nov 9, 2023

hosekadam commented Nov 15, 2023

hosekadam commented Nov 24, 2023

Venefilyn Jan 8, 2024

Venefilyn Jan 8, 2024

hosekadam commented Jan 11, 2024

Venefilyn commented Jan 11, 2024

[RHELC-1153] Port the restorable file to work with backup controller #935

[RHELC-1153] Port the restorable file to work with backup controller #935

Conversation

hosekadam commented Sep 26, 2023

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

jochapma left a comment

Choose a reason for hiding this comment

jochapma left a comment

Choose a reason for hiding this comment

hosekadam commented Oct 10, 2023

pr-watson left a comment

Choose a reason for hiding this comment

Venefilyn Oct 31, 2023

Choose a reason for hiding this comment

hosekadam Nov 6, 2023

Choose a reason for hiding this comment

Venefilyn Nov 9, 2023

Choose a reason for hiding this comment

hosekadam commented Nov 15, 2023

hosekadam commented Nov 24, 2023

Venefilyn Jan 8, 2024

Choose a reason for hiding this comment

Venefilyn Jan 8, 2024

Choose a reason for hiding this comment

hosekadam commented Jan 11, 2024

Venefilyn commented Jan 11, 2024

codecov bot commented Sep 27, 2023 •

edited

Loading