Feat/backup rather than hash check (#30)

* Back up existing required files rather than checking the hash in conversion script Signed-off-by: Andrea Waltlova <[email protected]> * Back up existing required files rather than checking the hash in pre-conversion script Signed-off-by: Andrea Waltlova <[email protected]> * Satisfy pre-commit Signed-off-by: Andrea Waltlova <[email protected]> * Let backup restore function throw IOError, it will be catched in main Signed-off-by: Andrea Waltlova <[email protected]> * Add attribute allowing us to keep required files when conversion successfull Signed-off-by: Andrea Waltlova <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Andrea Waltlova <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
oamg · Nov 22, 2023 · a969ea1 · a969ea1
1 parent 8b96685
commit a969ea1
Show file tree

Hide file tree

Showing 13 changed files with 266 additions and 294 deletions.
diff --git a/scripts/conversion_script.py b/scripts/conversion_script.py
@@ -1,4 +1,3 @@
-import hashlib
 import json
 import os
 import subprocess
@@ -26,11 +25,10 @@
 class RequiredFile(object):
     """Holds data about files needed to download convert2rhel"""
 
-    def __init__(self, path="", host=""):
+    def __init__(self, path="", host="", keep=False):
         self.path = path
         self.host = host
-        self.sha512_on_system = None
-        self.is_file_present = False
+        self.keep = keep
 
 
 class ProcessError(Exception):
@@ -151,13 +149,14 @@ def gather_textual_report():
     return data
 
 
-def generate_report_message(highest_status):
+def generate_report_message(highest_status, gpg_key_file):
     """Generate a report message based on the status severity."""
     message = ""
     alert = False
 
     if STATUS_CODE[highest_status] <= STATUS_CODE["WARNING"]:
         message = "No problems found. The system was converted successfully."
+        gpg_key_file.keep = True
 
     if STATUS_CODE[highest_status] > STATUS_CODE["WARNING"]:
         message = "The conversion cannot proceed. You must resolve existing issues to perform the conversion."
@@ -170,34 +169,19 @@ def setup_convert2rhel(required_files):
     """Setup convert2rhel tool by downloading the required files."""
     print("Downloading required files.")
     for required_file in required_files:
+        _create_or_restore_backup_file(required_file)
         response = urlopen(required_file.host)
         data = response.read()
-        downloaded_file_sha512 = hashlib.sha512(data)
 
-        if os.path.exists(required_file.path):
-            print(
-                "File '%s' is already present on the system. Downloading a copy in order to check if they are the same."
-                % required_file.path
-            )
-            if (
-                downloaded_file_sha512.hexdigest()
-                != required_file.sha512_on_system.hexdigest()
-            ):
-                raise ProcessError(
-                    message="Hash mismatch between the downloaded file and the one present on the system.",
-                    report="File '%s' present on the system does not match the one downloaded. Stopping the execution."
-                    % required_file.path,
-                )
-        else:
-            directory = os.path.dirname(required_file.path)
-            if not os.path.exists(directory):
-                print("Creating directory at '%s'" % directory)
-                os.makedirs(directory, mode=0o755)
-
-            print("Writing file to destination: '%s'" % required_file.path)
-            with open(required_file.path, mode="w") as handler:
-                handler.write(data)
-                os.chmod(required_file.path, 0o644)
+        directory = os.path.dirname(required_file.path)
+        if not os.path.exists(directory):
+            print("Creating directory at '%s'" % directory)
+            os.makedirs(directory, mode=0o755)
+
+        print("Writing file to destination: '%s'" % required_file.path)
+        with open(required_file.path, mode="w") as handler:
+            handler.write(data)
+            os.chmod(required_file.path, 0o644)
 
 
 # Code taken from
@@ -295,32 +279,32 @@ def cleanup(required_files):
     not something that was downloaded by the script.
     """
     for required_file in required_files:
-        if not required_file.is_file_present and os.path.exists(required_file.path):
+        if required_file.keep:
+            continue
+        if os.path.exists(required_file.path):
             print(
                 "Removing the file '%s' as it was previously downloaded."
                 % required_file.path
             )
             os.remove(required_file.path)
-            continue
+        _create_or_restore_backup_file(required_file)
 
+
+def _create_or_restore_backup_file(required_file):
+    """
+    Either creates or restores backup files (rename in both cases).
+    """
+    suffix = ".backup"
+    if os.path.exists(required_file.path + suffix):
+        print("Restoring backed up file %s." % (required_file.path))
+        os.rename(required_file.path + suffix, required_file.path)
+        return
+    if os.path.exists(required_file.path):
         print(
-            "File '%s' was present on the system before the execution. Skipping the removal."
-            % required_file.path
+            "File %s already present on system, backing up to %s."
+            % (required_file.path, required_file.path + suffix)
         )
-
-
-def verify_required_files_are_present(required_files):
-    """Verify if the required files are already present on the system."""
-    print("Checking if required files are present on the system.")
-    for required_file in required_files:
-        # Avoid race conditions
-        try:
-            print("Checking for file %s" % required_file.path)
-            with open(required_file.path, mode="r") as handler:
-                required_file.sha512_on_system = hashlib.sha512(handler.read())
-                required_file.is_file_present = True
-        except (IOError, OSError):
-            required_file.is_file_present = False
+        os.rename(required_file.path, required_file.path + ".backup")
 
 
 def _generate_message_key(message, action_id):
@@ -432,11 +416,13 @@ def update_insights_inventory():
 def main():
     """Main entrypoint for the script."""
     output = OutputCollector()
+    gpg_key_file = RequiredFile(
+        path="/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release",
+        host="https://www.redhat.com/security/data/fd431d51.txt",
+    )
+
     required_files = [
-        RequiredFile(
-            path="/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release",
-            host="https://www.redhat.com/security/data/fd431d51.txt",
-        ),
+        gpg_key_file,
         RequiredFile(
             path="/etc/yum.repos.d/convert2rhel.repo",
             host="https://ftp.redhat.com/redhat/convert2rhel/7/convert2rhel.repo",
@@ -445,7 +431,6 @@ def main():
 
     try:
         # Setup Convert2RHEL to be executed.
-        verify_required_files_are_present(required_files)
         setup_convert2rhel(required_files)
         install_convert2rhel()
         run_convert2rhel()
@@ -462,7 +447,9 @@ def main():
 
         # Generate report message and transform the raw data into entries for
         # Insights.
-        output.message, output.alert = generate_report_message(highest_level)
+        output.message, output.alert = generate_report_message(
+            highest_level, gpg_key_file
+        )
         output.entries = transform_raw_data(data)
         update_insights_inventory()
         print("Conversion script finish successfully!")

diff --git a/scripts/preconversion_assessment_script.py b/scripts/preconversion_assessment_script.py
@@ -1,4 +1,3 @@
-import hashlib
 import json
 import os
 import subprocess
@@ -29,8 +28,6 @@ class RequiredFile(object):
     def __init__(self, path="", host=""):
         self.path = path
         self.host = host
-        self.sha512_on_system = None
-        self.is_file_present = False
 
 
 class ProcessError(Exception):
@@ -167,34 +164,19 @@ def setup_convert2rhel(required_files):
     """Setup convert2rhel tool by downloading the required files."""
     print("Downloading required files.")
     for required_file in required_files:
+        _create_or_restore_backup_file(required_file)
         response = urlopen(required_file.host)
         data = response.read()
-        downloaded_file_sha512 = hashlib.sha512(data)
 
-        if os.path.exists(required_file.path):
-            print(
-                "File '%s' is already present on the system. Downloading a copy in order to check if they are the same."
-                % required_file.path
-            )
-            if (
-                downloaded_file_sha512.hexdigest()
-                != required_file.sha512_on_system.hexdigest()
-            ):
-                raise ProcessError(
-                    message="Hash mismatch between the downloaded file and the one present on the system.",
-                    report="File '%s' present on the system does not match the one downloaded. Stopping the execution."
-                    % required_file.path,
-                )
-        else:
-            directory = os.path.dirname(required_file.path)
-            if not os.path.exists(directory):
-                print("Creating directory at '%s'" % directory)
-                os.makedirs(directory, mode=0o755)
-
-            print("Writing file to destination: '%s'" % required_file.path)
-            with open(required_file.path, mode="w") as handler:
-                handler.write(data)
-                os.chmod(required_file.path, 0o644)
+        directory = os.path.dirname(required_file.path)
+        if not os.path.exists(directory):
+            print("Creating directory at '%s'" % directory)
+            os.makedirs(directory, mode=0o755)
+
+        print("Writing file to destination: '%s'" % required_file.path)
+        with open(required_file.path, mode="w") as handler:
+            handler.write(data)
+            os.chmod(required_file.path, 0o644)
 
 
 # Code taken from
@@ -294,32 +276,30 @@ def cleanup(required_files):
     not something that was downloaded by the script.
     """
     for required_file in required_files:
-        if not required_file.is_file_present and os.path.exists(required_file.path):
+        if os.path.exists(required_file.path):
             print(
                 "Removing the file '%s' as it was previously downloaded."
                 % required_file.path
             )
             os.remove(required_file.path)
-            continue
+        _create_or_restore_backup_file(required_file)
 
+
+def _create_or_restore_backup_file(required_file):
+    """
+    Either creates or restores backup files (rename in both cases).
+    """
+    suffix = ".backup"
+    if os.path.exists(required_file.path + suffix):
+        print("Restoring backed up file %s." % (required_file.path))
+        os.rename(required_file.path + suffix, required_file.path)
+        return
+    if os.path.exists(required_file.path):
         print(
-            "File '%s' was present on the system before the execution. Skipping the removal."
-            % required_file.path
+            "File %s already present on system, backing up to %s."
+            % (required_file.path, required_file.path + suffix)
         )
-
-
-def verify_required_files_are_present(required_files):
-    """Verify if the required files are already present on the system."""
-    print("Checking if required files are present on the system.")
-    for required_file in required_files:
-        # Avoid race conditions
-        try:
-            print("Checking for file %s" % required_file.path)
-            with open(required_file.path, mode="r") as handler:
-                required_file.sha512_on_system = hashlib.sha512(handler.read())
-                required_file.is_file_present = True
-        except (IOError, OSError):
-            required_file.is_file_present = False
+        os.rename(required_file.path, required_file.path + ".backup")
 
 
 def _generate_message_key(message, action_id):
@@ -429,7 +409,6 @@ def main():
 
     try:
         # Setup Convert2RHEL to be executed.
-        verify_required_files_are_present(required_files)
         setup_convert2rhel(required_files)
         install_convert2rhel()
         run_convert2rhel()

diff --git a/tests/conversion_script/test_backup_restore_files.py b/tests/conversion_script/test_backup_restore_files.py
@@ -0,0 +1,41 @@
+import pytest
+from mock import patch
+
+from scripts.conversion_script import (
+    RequiredFile,
+    _create_or_restore_backup_file,
+)
+
+
+@patch("scripts.conversion_script.os")
+def test_backup_existing_file(mock_os):
+    filepath = "/path/to/file"
+    required_file = RequiredFile(path=filepath)
+    mock_os.path.exists.side_effect = [False, True]
+
+    _create_or_restore_backup_file(required_file)
+
+    mock_os.path.exists.assert_called_with(filepath)
+    mock_os.rename.assert_called_once_with(filepath, filepath + ".backup")
+
+
+@patch("scripts.conversion_script.os")
+def test_restore_existing_file(mock_os):
+    filepath = "/path/to/file"
+    required_file = RequiredFile(path=filepath)
+    mock_os.path.exists.side_effect = [True]
+
+    _create_or_restore_backup_file(required_file)
+
+    mock_os.path.exists.assert_called_with(filepath + ".backup")
+    mock_os.rename.assert_called_once_with(filepath + ".backup", filepath)
+
+
+@patch("scripts.conversion_script.os")
+def test_ioerror_restore_or_backup(mock_os):
+    filepath = "/path/to/file"
+    required_file = RequiredFile(path=filepath)
+    mock_os.path.exists.side_effect = IOError
+
+    with pytest.raises(IOError):
+        _create_or_restore_backup_file(required_file)
diff --git a/tests/conversion_script/test_cleanup.py b/tests/conversion_script/test_cleanup.py
@@ -5,15 +5,33 @@
 
 @patch("os.path.exists", side_effect=Mock())
 @patch("os.remove", side_effect=Mock())
-def test_cleanup_with_file_to_remove(mock_remove, mock_exists):
+@patch("scripts.conversion_script._create_or_restore_backup_file")
+def test_cleanup_with_file_to_remove(mock_restore, mock_remove, mock_exists):
     """Only downloaded files are removed."""
 
     present_file = RequiredFile("/already/present")
-    present_file.is_file_present = True
-    downloaded_file = RequiredFile("/downloaded")
-    required_files = [present_file, downloaded_file]
+    required_files = [present_file]
 
     cleanup(required_files)
 
+    # For removal of file, then two checks in backup function
     assert mock_exists.call_count == 1
     assert mock_remove.call_count == 1
+    assert mock_restore.call_count == 1
+
+
+@patch("os.path.exists", side_effect=Mock())
+@patch("os.remove", side_effect=Mock())
+@patch("scripts.conversion_script._create_or_restore_backup_file")
+def test_cleanup_with_file_to_keep(mock_restore, mock_remove, mock_exists):
+    """Only downloaded files are removed."""
+
+    keep_downloaded_file = RequiredFile("/download/keep", keep=True)
+    required_files = [keep_downloaded_file]
+
+    cleanup(required_files)
+
+    # For removal of file, then two checks in backup function
+    assert mock_exists.call_count == 0
+    assert mock_remove.call_count == 0
+    assert mock_restore.call_count == 0