From 6ba276a4ba0de676f652ab0b2932d09278a83d77 Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Mon, 26 May 2025 20:30:33 +0200 Subject: [PATCH 1/7] Add a backup and restore step on the netplans directory --- providers/base/bin/wifi_nmcli_test.py | 87 ++++++++ .../tests/test_wifi_client_test_netplan.py | 1 + providers/base/tests/test_wifi_nmcli_test.py | 209 ++++++++++++++++++ 3 files changed, 297 insertions(+) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index 61b4fb72c1..a8098403a0 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -18,12 +18,17 @@ import subprocess as sp import sys import shlex +from pathlib import Path +import shutil +import glob +import tempfile from packaging import version as version_parser from checkbox_support.helpers.retry import retry from gateway_ping_test import ping +NETPLAN_DIR = "/lib/netplan" print = functools.partial(print, flush=True) @@ -386,6 +391,80 @@ def parser_args(): return args +def backup_netplan_files(backup_dir: str, netplan_dir: str): + """ + Backup netplan YAML files from /etc/netplan/ to a + temporary directory, if there are. + """ + + # Find all netplan YAML files + yaml_files = glob.glob(os.path.join(netplan_dir, "*.yaml")) + + if not yaml_files: + print("No netplan YAML files found") + + # Create temporary directory + Path(backup_dir).mkdir(parents=True, exist_ok=True) + + # Copy each file to temp directory + for yaml_file in yaml_files: + filename = os.path.basename(yaml_file) + temp_path = os.path.join(backup_dir, filename) + shutil.copy2(yaml_file, temp_path) + # Then copy ownership + st = os.stat(yaml_file) + os.chown(temp_path, st.st_uid, st.st_gid) + print("Backed up: {} -> {}".format(yaml_file, temp_path)) + + print("Netplan files backed up to: {}", backup_dir) + +def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: + """ + Restore netplan YAML files from backup directory to /etc/netplan/. + + Returns: + bool: True if restoration successful, False otherwise + """ + if not backup_dir or not os.path.exists(backup_dir): + print("Backup directory does not exist: {}".format(netplan_dir)) + return False + + # Clean up existing netplan files first + existing_files = glob.glob(os.path.join(netplan_dir, "*.yaml")) + for existing_file in existing_files: + os.remove(existing_file) + print("Removed: {}".format(existing_file)) + + # Find all YAML files in backup directory + backup_files = glob.glob(os.path.join(backup_dir, "*.yaml")) + + if not backup_files: + print("No netplan files found in backup directory") + return False + + # Restore each file + for backup_file in backup_files: + filename = os.path.basename(backup_file) + target_path = os.path.join(netplan_dir, filename) + shutil.copy2(backup_file, target_path) + # Then copy ownership + st = os.stat(backup_file) + os.chown(target_path, st.st_uid, st.st_gid) + print("Restored: {} -> {}".format(backup_file, target_path)) + + print("Netplan files restored successfully") + return True + +def cleanup_netplan_backup(backup_dir: str): + """ + Clean up temporary backup directory. + """ + if not os.path.exists(backup_dir): + return + + shutil.rmtree(backup_dir) + print("Cleaned up backup directory: {}".format(backup_dir)) + @retry(max_attempts=5, delay=60) def main(): args = parser_args() @@ -408,6 +487,12 @@ def main(): ) if args.func: + # backup the test plans, because nmcli corrupts them + # and debsums will complain afterwards + # This is ugly. Ideally, nmcli should be patched instead + temp_dir = tempfile.TemporaryDirectory() + backup_netplan_files(temp_dir, NETPLAN_DIR) + delete_test_ap_ssid_connection() activated_uuid = get_nm_activate_connection() turn_down_nm_connections() @@ -423,6 +508,8 @@ def main(): finally: turn_up_connection(activated_uuid) delete_test_ap_ssid_connection() + restore_netplan_files(temp_dir, NETPLAN_DIR) + cleanup_netplan_backup(temp_dir) # cannot use temp_dir.cleanup to support old pythons if __name__ == "__main__": diff --git a/providers/base/tests/test_wifi_client_test_netplan.py b/providers/base/tests/test_wifi_client_test_netplan.py index 7a07c8c5f4..1414cb72c6 100644 --- a/providers/base/tests/test_wifi_client_test_netplan.py +++ b/providers/base/tests/test_wifi_client_test_netplan.py @@ -24,6 +24,7 @@ import io import sys from unittest.mock import call + from wifi_client_test_netplan import ( netplan_renderer, check_and_get_renderer, diff --git a/providers/base/tests/test_wifi_nmcli_test.py b/providers/base/tests/test_wifi_nmcli_test.py index de860d5ce9..139a26f29b 100644 --- a/providers/base/tests/test_wifi_nmcli_test.py +++ b/providers/base/tests/test_wifi_nmcli_test.py @@ -20,6 +20,9 @@ import unittest from subprocess import CalledProcessError from unittest.mock import patch, call, MagicMock +from pathlib import Path +import shutil +import tempfile from checkbox_support.helpers.retry import mock_retry @@ -42,6 +45,9 @@ perform_ping_test, parser_args, main, + restore_netplan_files, + backup_netplan_files, + cleanup_netplan_backup, ) @@ -606,6 +612,9 @@ def test_main_open_no_aps_found( "TestSSID": {"Chan": "11", "Freq": "2462", "Signal": "80"}, }, ) + @patch("wifi_nmcli_test.backup_netplan_files") + @patch("wifi_nmcli_test.restore_netplan_files") + @patch("wifi_nmcli_test.cleanup_netplan_backup") @patch("wifi_nmcli_test.open_connection", return_value=0) @patch( "wifi_nmcli_test.sys.argv", @@ -616,5 +625,205 @@ def test_main_open_aps_found( list_aps_mock, get_nm_activate_connection_mock, mock_open_connection, + mock_cleanup_back, + mock_rest_back, + mock_cr_back, ): main() + + +class TestNetplanBackupFunctions(unittest.TestCase): + def setUp(self): + self.TEST_BACKUP_DIR = tempfile.TemporaryDirectory() + self.TEST_NETPLAN_DIR = tempfile.TemporaryDirectory() + """Set up test fixtures before each test method.""" + Path(self.TEST_BACKUP_DIR).mkdir(parents=True, exist_ok=True) + Path(self.TEST_NETPLAN_DIR).mkdir(parents=True, exist_ok=True) + + @patch("glob.glob") + @patch("builtins.print") + def test_backup_netplan_files_no_files_found(self, mock_print, mock_glob): + """Test backup when no YAML files are found.""" + mock_glob.return_value = [] + + backup_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) + + @patch("os.chown") + @patch("os.stat") + @patch("glob.glob") + @patch("shutil.copy2") + @patch("pathlib.Path.mkdir") + @patch("builtins.print") + def test_backup_netplan_files_success( + self, + mock_print, + mock_mkdir, + mock_copy2, + mock_glob, + mock_stat, + mock_chown, + ): + """Test successful backup of netplan files.""" + mock_glob.return_value = [ + self.TEST_NETPLAN_DIR + "/config1.yaml", + self.TEST_NETPLAN_DIR + "/config2.yaml", + ] + + backup_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) + + self.assertEqual(mock_copy2.call_count, 2) + mock_mkdir.assert_called_once_with(parents=True, exist_ok=True) + + @patch("os.chown") + @patch("os.stat") + @patch("os.path.exists") + @patch("glob.glob") + @patch("os.remove") + @patch("os.makedirs") + @patch("shutil.copy2") + @patch("builtins.print") + def test_restore_netplan_files_success( + self, + mock_print, + mock_copy2, + mock_makedirs, + mock_remove, + mock_glob, + mock_exists, + mock_stat, + mock_chown, + ): + """Test successful restore of netplan files.""" + mock_exists.return_value = True + + mock_glob.side_effect = [ + [], + [], + ] + + result = restore_netplan_files(None, self.TEST_NETPLAN_DIR) + result = restore_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) + + mock_glob.side_effect = [ + [self.TEST_NETPLAN_DIR + "/old1.yaml"], # Existing files to remove + [ + "{}/config1.yaml".format(self.TEST_BACKUP_DIR), + "{}/config2.yaml".format(self.TEST_BACKUP_DIR), + ], # Backup files + ] + + result = restore_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) + + self.assertTrue(result) + mock_remove.assert_called_once_with( + self.TEST_NETPLAN_DIR + "/old1.yaml" + ) + self.assertEqual(mock_copy2.call_count, 2) + + @patch("os.chown") + @patch("os.stat") + @patch("os.path.exists") + @patch("glob.glob") + @patch("os.remove") + @patch("shutil.copy2") + @patch("builtins.print") + def test_restore_netplan_files_copy_error( + self, + mock_print, + mock_copy2, + mock_remove, + mock_glob, + mock_exists, + mock_stat, + mock_chown, + ): + """Test restore when copy operation fails.""" + mock_exists.return_value = True + mock_glob.side_effect = [ + [], + ["{}/config1.yaml".format(self.TEST_BACKUP_DIR)], + ] + mock_copy2.side_effect = OSError("Permission denied") + + result = restore_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) + + self.assertFalse(result) + + @patch("os.path.exists") + @patch("builtins.print") + def test_cleanup_backup_not_exists(self, mock_print, mock_exists): + """Test cleanup when backup directory doesn't exist.""" + mock_exists.return_value = False + cleanup_netplan_backup(self.TEST_BACKUP_DIR) + + @patch("os.path.exists") + @patch("shutil.rmtree") + @patch("builtins.print") + def test_cleanup_backup_success( + self, mock_print, mock_rmtree, mock_exists + ): + """Test successful cleanup of backup directory.""" + mock_exists.return_value = True + + cleanup_netplan_backup(self.TEST_BACKUP_DIR) + + mock_rmtree.assert_called_once_with(self.TEST_BACKUP_DIR) + + @patch("os.path.exists") + @patch("shutil.rmtree") + @patch("builtins.print") + def test_cleanup_backup_error(self, mock_print, mock_rmtree, mock_exists): + """Test cleanup when rmtree operation fails.""" + mock_exists.return_value = True + mock_rmtree.side_effect = OSError("Permission denied") + + cleanup_netplan_backup(self.TEST_BACKUP_DIR) + + @patch("os.path.exists") + @patch("glob.glob") + @patch("os.remove") + @patch("builtins.print") + def test_restore_netplan_files_remove_error( + self, mock_print, mock_remove, mock_glob, mock_exists + ): + """Test restore when removing existing files fails.""" + mock_exists.return_value = True + mock_glob.side_effect = [ + [self.TEST_NETPLAN_DIR + "/old1.yaml"], # Existing files to remove + ["{}/config1.yaml".format(self.TEST_BACKUP_DIR)], # Backup files + ] + mock_remove.side_effect = OSError("Permission denied") + with self.assertRaises(OSError): + result = restore_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) + + @patch("os.path.exists") + @patch("glob.glob") + @patch("os.makedirs") + @patch("builtins.print") + def test_restore_netplan_files_makedirs_error( + self, mock_print, mock_makedirs, mock_glob, mock_exists + ): + """Test restore when makedirs operation fails.""" + mock_exists.return_value = True + mock_glob.side_effect = [ + [], + ["{}/config1.yaml".format(self.TEST_BACKUP_DIR)], + ] + mock_makedirs.side_effect = OSError("Permission denied") + + with self.assertRaises(FileNotFoundError): + restore_netplan_files( + self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + ) \ No newline at end of file From 23df24bd0335cc3176875e2c2a6bf963e4445a33 Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Wed, 25 Jun 2025 17:25:50 +0200 Subject: [PATCH 2/7] Fix tests --- providers/base/bin/wifi_nmcli_test.py | 6 +- providers/base/tests/test_wifi_nmcli_test.py | 88 ++++++-------------- 2 files changed, 30 insertions(+), 64 deletions(-) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index a8098403a0..d34269dc5c 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -418,6 +418,7 @@ def backup_netplan_files(backup_dir: str, netplan_dir: str): print("Netplan files backed up to: {}", backup_dir) + def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: """ Restore netplan YAML files from backup directory to /etc/netplan/. @@ -455,6 +456,7 @@ def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: print("Netplan files restored successfully") return True + def cleanup_netplan_backup(backup_dir: str): """ Clean up temporary backup directory. @@ -465,6 +467,7 @@ def cleanup_netplan_backup(backup_dir: str): shutil.rmtree(backup_dir) print("Cleaned up backup directory: {}".format(backup_dir)) + @retry(max_attempts=5, delay=60) def main(): args = parser_args() @@ -509,7 +512,8 @@ def main(): turn_up_connection(activated_uuid) delete_test_ap_ssid_connection() restore_netplan_files(temp_dir, NETPLAN_DIR) - cleanup_netplan_backup(temp_dir) # cannot use temp_dir.cleanup to support old pythons + # cannot use temp_dir.cleanup to support old pythons + cleanup_netplan_backup(temp_dir) if __name__ == "__main__": diff --git a/providers/base/tests/test_wifi_nmcli_test.py b/providers/base/tests/test_wifi_nmcli_test.py index 139a26f29b..eac140254e 100644 --- a/providers/base/tests/test_wifi_nmcli_test.py +++ b/providers/base/tests/test_wifi_nmcli_test.py @@ -637,8 +637,8 @@ def setUp(self): self.TEST_BACKUP_DIR = tempfile.TemporaryDirectory() self.TEST_NETPLAN_DIR = tempfile.TemporaryDirectory() """Set up test fixtures before each test method.""" - Path(self.TEST_BACKUP_DIR).mkdir(parents=True, exist_ok=True) - Path(self.TEST_NETPLAN_DIR).mkdir(parents=True, exist_ok=True) + Path(str(self.TEST_BACKUP_DIR)).mkdir(parents=True, exist_ok=True) + Path(str(self.TEST_NETPLAN_DIR)).mkdir(parents=True, exist_ok=True) @patch("glob.glob") @patch("builtins.print") @@ -647,7 +647,7 @@ def test_backup_netplan_files_no_files_found(self, mock_print, mock_glob): mock_glob.return_value = [] backup_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) ) @patch("os.chown") @@ -667,12 +667,12 @@ def test_backup_netplan_files_success( ): """Test successful backup of netplan files.""" mock_glob.return_value = [ - self.TEST_NETPLAN_DIR + "/config1.yaml", - self.TEST_NETPLAN_DIR + "/config2.yaml", + str(self.TEST_NETPLAN_DIR) + "/config1.yaml", + str(self.TEST_NETPLAN_DIR) + "/config2.yaml", ] backup_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) ) self.assertEqual(mock_copy2.call_count, 2) @@ -705,66 +705,36 @@ def test_restore_netplan_files_success( [], ] - result = restore_netplan_files(None, self.TEST_NETPLAN_DIR) + result = restore_netplan_files(None, str(self.TEST_NETPLAN_DIR)) result = restore_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) ) mock_glob.side_effect = [ - [self.TEST_NETPLAN_DIR + "/old1.yaml"], # Existing files to remove + # Existing files to remove + [str(self.TEST_NETPLAN_DIR) + "/old1.yaml"], [ - "{}/config1.yaml".format(self.TEST_BACKUP_DIR), - "{}/config2.yaml".format(self.TEST_BACKUP_DIR), + "{}/config1.yaml".format(str(self.TEST_BACKUP_DIR)), + "{}/config2.yaml".format(str(self.TEST_BACKUP_DIR)), ], # Backup files ] result = restore_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) ) self.assertTrue(result) mock_remove.assert_called_once_with( - self.TEST_NETPLAN_DIR + "/old1.yaml" + str(self.TEST_NETPLAN_DIR) + "/old1.yaml" ) self.assertEqual(mock_copy2.call_count, 2) - @patch("os.chown") - @patch("os.stat") - @patch("os.path.exists") - @patch("glob.glob") - @patch("os.remove") - @patch("shutil.copy2") - @patch("builtins.print") - def test_restore_netplan_files_copy_error( - self, - mock_print, - mock_copy2, - mock_remove, - mock_glob, - mock_exists, - mock_stat, - mock_chown, - ): - """Test restore when copy operation fails.""" - mock_exists.return_value = True - mock_glob.side_effect = [ - [], - ["{}/config1.yaml".format(self.TEST_BACKUP_DIR)], - ] - mock_copy2.side_effect = OSError("Permission denied") - - result = restore_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR - ) - - self.assertFalse(result) - @patch("os.path.exists") @patch("builtins.print") def test_cleanup_backup_not_exists(self, mock_print, mock_exists): """Test cleanup when backup directory doesn't exist.""" mock_exists.return_value = False - cleanup_netplan_backup(self.TEST_BACKUP_DIR) + cleanup_netplan_backup(str(self.TEST_BACKUP_DIR)) @patch("os.path.exists") @patch("shutil.rmtree") @@ -775,19 +745,9 @@ def test_cleanup_backup_success( """Test successful cleanup of backup directory.""" mock_exists.return_value = True - cleanup_netplan_backup(self.TEST_BACKUP_DIR) - - mock_rmtree.assert_called_once_with(self.TEST_BACKUP_DIR) - - @patch("os.path.exists") - @patch("shutil.rmtree") - @patch("builtins.print") - def test_cleanup_backup_error(self, mock_print, mock_rmtree, mock_exists): - """Test cleanup when rmtree operation fails.""" - mock_exists.return_value = True - mock_rmtree.side_effect = OSError("Permission denied") + cleanup_netplan_backup(str(self.TEST_BACKUP_DIR)) - cleanup_netplan_backup(self.TEST_BACKUP_DIR) + mock_rmtree.assert_called_once_with(str(self.TEST_BACKUP_DIR)) @patch("os.path.exists") @patch("glob.glob") @@ -799,13 +759,15 @@ def test_restore_netplan_files_remove_error( """Test restore when removing existing files fails.""" mock_exists.return_value = True mock_glob.side_effect = [ - [self.TEST_NETPLAN_DIR + "/old1.yaml"], # Existing files to remove - ["{}/config1.yaml".format(self.TEST_BACKUP_DIR)], # Backup files + # Existing files to remove + [str(self.TEST_NETPLAN_DIR) + "/old1.yaml"], + # Backup files + ["{}/config1.yaml".format(str(self.TEST_BACKUP_DIR))], ] mock_remove.side_effect = OSError("Permission denied") with self.assertRaises(OSError): result = restore_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR + str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) ) @patch("os.path.exists") @@ -819,11 +781,11 @@ def test_restore_netplan_files_makedirs_error( mock_exists.return_value = True mock_glob.side_effect = [ [], - ["{}/config1.yaml".format(self.TEST_BACKUP_DIR)], + ["{}/config1.yaml".format(str(self.TEST_BACKUP_DIR))], ] mock_makedirs.side_effect = OSError("Permission denied") with self.assertRaises(FileNotFoundError): restore_netplan_files( - self.TEST_BACKUP_DIR, self.TEST_NETPLAN_DIR - ) \ No newline at end of file + str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + ) From 55b564ec7fad2ef1925f149695b060345efe67e5 Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Wed, 25 Jun 2025 20:56:55 +0200 Subject: [PATCH 3/7] Fix temporary directories --- providers/base/bin/wifi_nmcli_test.py | 7 +-- providers/base/tests/test_wifi_nmcli_test.py | 47 ++++++++++---------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index d34269dc5c..f8cb27b51e 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -494,7 +494,8 @@ def main(): # and debsums will complain afterwards # This is ugly. Ideally, nmcli should be patched instead temp_dir = tempfile.TemporaryDirectory() - backup_netplan_files(temp_dir, NETPLAN_DIR) + + backup_netplan_files(str(temp_dir.name), NETPLAN_DIR) delete_test_ap_ssid_connection() activated_uuid = get_nm_activate_connection() @@ -511,9 +512,9 @@ def main(): finally: turn_up_connection(activated_uuid) delete_test_ap_ssid_connection() - restore_netplan_files(temp_dir, NETPLAN_DIR) + restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) # cannot use temp_dir.cleanup to support old pythons - cleanup_netplan_backup(temp_dir) + cleanup_netplan_backup(str(temp_dir.name)) if __name__ == "__main__": diff --git a/providers/base/tests/test_wifi_nmcli_test.py b/providers/base/tests/test_wifi_nmcli_test.py index eac140254e..227c06c4af 100644 --- a/providers/base/tests/test_wifi_nmcli_test.py +++ b/providers/base/tests/test_wifi_nmcli_test.py @@ -634,11 +634,12 @@ def test_main_open_aps_found( class TestNetplanBackupFunctions(unittest.TestCase): def setUp(self): - self.TEST_BACKUP_DIR = tempfile.TemporaryDirectory() - self.TEST_NETPLAN_DIR = tempfile.TemporaryDirectory() + self.TEST_BACKUP_DIR.name = tempfile.TemporaryDirectory() + self.TEST_NETPLAN_DIR.name = tempfile.TemporaryDirectory() """Set up test fixtures before each test method.""" - Path(str(self.TEST_BACKUP_DIR)).mkdir(parents=True, exist_ok=True) - Path(str(self.TEST_NETPLAN_DIR)).mkdir(parents=True, exist_ok=True) + Path(str(self.TEST_BACKUP_DIR.name)).mkdir(parents=True, exist_ok=True) + Path(str(self.TEST_NETPLAN_DIR.name)).mkdir( + parents=True, exist_ok=True) @patch("glob.glob") @patch("builtins.print") @@ -647,7 +648,7 @@ def test_backup_netplan_files_no_files_found(self, mock_print, mock_glob): mock_glob.return_value = [] backup_netplan_files( - str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) @patch("os.chown") @@ -667,12 +668,12 @@ def test_backup_netplan_files_success( ): """Test successful backup of netplan files.""" mock_glob.return_value = [ - str(self.TEST_NETPLAN_DIR) + "/config1.yaml", - str(self.TEST_NETPLAN_DIR) + "/config2.yaml", + str(self.TEST_NETPLAN_DIR.name) + "/config1.yaml", + str(self.TEST_NETPLAN_DIR.name) + "/config2.yaml", ] backup_netplan_files( - str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) self.assertEqual(mock_copy2.call_count, 2) @@ -705,27 +706,27 @@ def test_restore_netplan_files_success( [], ] - result = restore_netplan_files(None, str(self.TEST_NETPLAN_DIR)) + result = restore_netplan_files(None, str(self.TEST_NETPLAN_DIR.name)) result = restore_netplan_files( - str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) mock_glob.side_effect = [ # Existing files to remove - [str(self.TEST_NETPLAN_DIR) + "/old1.yaml"], + [str(self.TEST_NETPLAN_DIR.name) + "/old1.yaml"], [ - "{}/config1.yaml".format(str(self.TEST_BACKUP_DIR)), - "{}/config2.yaml".format(str(self.TEST_BACKUP_DIR)), + "{}/config1.yaml".format(str(self.TEST_BACKUP_DIR.name)), + "{}/config2.yaml".format(str(self.TEST_BACKUP_DIR.name)), ], # Backup files ] result = restore_netplan_files( - str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) self.assertTrue(result) mock_remove.assert_called_once_with( - str(self.TEST_NETPLAN_DIR) + "/old1.yaml" + str(self.TEST_NETPLAN_DIR.name) + "/old1.yaml" ) self.assertEqual(mock_copy2.call_count, 2) @@ -734,7 +735,7 @@ def test_restore_netplan_files_success( def test_cleanup_backup_not_exists(self, mock_print, mock_exists): """Test cleanup when backup directory doesn't exist.""" mock_exists.return_value = False - cleanup_netplan_backup(str(self.TEST_BACKUP_DIR)) + cleanup_netplan_backup(str(self.TEST_BACKUP_DIR.name)) @patch("os.path.exists") @patch("shutil.rmtree") @@ -745,9 +746,9 @@ def test_cleanup_backup_success( """Test successful cleanup of backup directory.""" mock_exists.return_value = True - cleanup_netplan_backup(str(self.TEST_BACKUP_DIR)) + cleanup_netplan_backup(str(self.TEST_BACKUP_DIR.name)) - mock_rmtree.assert_called_once_with(str(self.TEST_BACKUP_DIR)) + mock_rmtree.assert_called_once_with(str(self.TEST_BACKUP_DIR.name)) @patch("os.path.exists") @patch("glob.glob") @@ -760,14 +761,14 @@ def test_restore_netplan_files_remove_error( mock_exists.return_value = True mock_glob.side_effect = [ # Existing files to remove - [str(self.TEST_NETPLAN_DIR) + "/old1.yaml"], + [str(self.TEST_NETPLAN_DIR.name) + "/old1.yaml"], # Backup files - ["{}/config1.yaml".format(str(self.TEST_BACKUP_DIR))], + ["{}/config1.yaml".format(str(self.TEST_BACKUP_DIR.name))], ] mock_remove.side_effect = OSError("Permission denied") with self.assertRaises(OSError): result = restore_netplan_files( - str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) @patch("os.path.exists") @@ -781,11 +782,11 @@ def test_restore_netplan_files_makedirs_error( mock_exists.return_value = True mock_glob.side_effect = [ [], - ["{}/config1.yaml".format(str(self.TEST_BACKUP_DIR))], + ["{}/config1.yaml".format(str(self.TEST_BACKUP_DIR.name))], ] mock_makedirs.side_effect = OSError("Permission denied") with self.assertRaises(FileNotFoundError): restore_netplan_files( - str(self.TEST_BACKUP_DIR), str(self.TEST_NETPLAN_DIR) + str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) From ce42a560c6a6de7511e02b278437eca5ae28093d Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Thu, 26 Jun 2025 13:36:22 +0200 Subject: [PATCH 4/7] Fix the backup/restore behavior of netplans, overall simplifications --- providers/base/bin/wifi_nmcli_test.py | 32 ++++++++------------ providers/base/tests/test_wifi_nmcli_test.py | 31 ++----------------- 2 files changed, 15 insertions(+), 48 deletions(-) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index f8cb27b51e..92cc57566e 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -457,19 +457,8 @@ def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: return True -def cleanup_netplan_backup(backup_dir: str): - """ - Clean up temporary backup directory. - """ - if not os.path.exists(backup_dir): - return - - shutil.rmtree(backup_dir) - print("Cleaned up backup directory: {}".format(backup_dir)) - - @retry(max_attempts=5, delay=60) -def main(): +def run(): args = parser_args() start_time = datetime.datetime.now() device_rescan() @@ -493,10 +482,6 @@ def main(): # backup the test plans, because nmcli corrupts them # and debsums will complain afterwards # This is ugly. Ideally, nmcli should be patched instead - temp_dir = tempfile.TemporaryDirectory() - - backup_netplan_files(str(temp_dir.name), NETPLAN_DIR) - delete_test_ap_ssid_connection() activated_uuid = get_nm_activate_connection() turn_down_nm_connections() @@ -512,9 +497,18 @@ def main(): finally: turn_up_connection(activated_uuid) delete_test_ap_ssid_connection() - restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) - # cannot use temp_dir.cleanup to support old pythons - cleanup_netplan_backup(str(temp_dir.name)) + + +def main(): + temp_dir = tempfile.TemporaryDirectory() + backup_netplan_files(str(temp_dir.name), NETPLAN_DIR) + try: + run() + except Exception: + restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) + raise + + restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) if __name__ == "__main__": diff --git a/providers/base/tests/test_wifi_nmcli_test.py b/providers/base/tests/test_wifi_nmcli_test.py index 227c06c4af..bcbd9914da 100644 --- a/providers/base/tests/test_wifi_nmcli_test.py +++ b/providers/base/tests/test_wifi_nmcli_test.py @@ -47,7 +47,6 @@ main, restore_netplan_files, backup_netplan_files, - cleanup_netplan_backup, ) @@ -614,7 +613,6 @@ def test_main_open_no_aps_found( ) @patch("wifi_nmcli_test.backup_netplan_files") @patch("wifi_nmcli_test.restore_netplan_files") - @patch("wifi_nmcli_test.cleanup_netplan_backup") @patch("wifi_nmcli_test.open_connection", return_value=0) @patch( "wifi_nmcli_test.sys.argv", @@ -625,7 +623,6 @@ def test_main_open_aps_found( list_aps_mock, get_nm_activate_connection_mock, mock_open_connection, - mock_cleanup_back, mock_rest_back, mock_cr_back, ): @@ -634,12 +631,8 @@ def test_main_open_aps_found( class TestNetplanBackupFunctions(unittest.TestCase): def setUp(self): - self.TEST_BACKUP_DIR.name = tempfile.TemporaryDirectory() - self.TEST_NETPLAN_DIR.name = tempfile.TemporaryDirectory() - """Set up test fixtures before each test method.""" - Path(str(self.TEST_BACKUP_DIR.name)).mkdir(parents=True, exist_ok=True) - Path(str(self.TEST_NETPLAN_DIR.name)).mkdir( - parents=True, exist_ok=True) + self.TEST_BACKUP_DIR = tempfile.TemporaryDirectory() + self.TEST_NETPLAN_DIR = tempfile.TemporaryDirectory() @patch("glob.glob") @patch("builtins.print") @@ -730,26 +723,6 @@ def test_restore_netplan_files_success( ) self.assertEqual(mock_copy2.call_count, 2) - @patch("os.path.exists") - @patch("builtins.print") - def test_cleanup_backup_not_exists(self, mock_print, mock_exists): - """Test cleanup when backup directory doesn't exist.""" - mock_exists.return_value = False - cleanup_netplan_backup(str(self.TEST_BACKUP_DIR.name)) - - @patch("os.path.exists") - @patch("shutil.rmtree") - @patch("builtins.print") - def test_cleanup_backup_success( - self, mock_print, mock_rmtree, mock_exists - ): - """Test successful cleanup of backup directory.""" - mock_exists.return_value = True - - cleanup_netplan_backup(str(self.TEST_BACKUP_DIR.name)) - - mock_rmtree.assert_called_once_with(str(self.TEST_BACKUP_DIR.name)) - @patch("os.path.exists") @patch("glob.glob") @patch("os.remove") From 96af48d08d8ebb23214a87b89af986384cbd7467 Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Thu, 26 Jun 2025 14:55:25 +0200 Subject: [PATCH 5/7] Modify a misplaced comment --- providers/base/bin/wifi_nmcli_test.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index 92cc57566e..f2c63a1173 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -479,9 +479,6 @@ def run(): ) if args.func: - # backup the test plans, because nmcli corrupts them - # and debsums will complain afterwards - # This is ugly. Ideally, nmcli should be patched instead delete_test_ap_ssid_connection() activated_uuid = get_nm_activate_connection() turn_down_nm_connections() @@ -500,8 +497,13 @@ def run(): def main(): + + # backup the test plans, because nmcli corrupts them + # and debsums will complain afterwards + # This is ugly. Ideally, nmcli should be patched instead temp_dir = tempfile.TemporaryDirectory() backup_netplan_files(str(temp_dir.name), NETPLAN_DIR) + try: run() except Exception: From 8dfcc8dcef34a2b0729b49881084c6d28488c942 Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Fri, 4 Jul 2025 12:41:38 +0200 Subject: [PATCH 6/7] Apply suggestions from review --- providers/base/bin/wifi_nmcli_test.py | 18 +++++++++--------- providers/base/tests/test_wifi_nmcli_test.py | 9 ++++----- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index f2c63a1173..8174fbf8b0 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -402,6 +402,7 @@ def backup_netplan_files(backup_dir: str, netplan_dir: str): if not yaml_files: print("No netplan YAML files found") + return # Create temporary directory Path(backup_dir).mkdir(parents=True, exist_ok=True) @@ -419,7 +420,7 @@ def backup_netplan_files(backup_dir: str, netplan_dir: str): print("Netplan files backed up to: {}", backup_dir) -def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: +def restore_netplan_files(backup_dir: str, netplan_dir: str): """ Restore netplan YAML files from backup directory to /etc/netplan/. @@ -428,7 +429,7 @@ def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: """ if not backup_dir or not os.path.exists(backup_dir): print("Backup directory does not exist: {}".format(netplan_dir)) - return False + return # Clean up existing netplan files first existing_files = glob.glob(os.path.join(netplan_dir, "*.yaml")) @@ -441,7 +442,7 @@ def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: if not backup_files: print("No netplan files found in backup directory") - return False + return # Restore each file for backup_file in backup_files: @@ -454,7 +455,7 @@ def restore_netplan_files(backup_dir: str, netplan_dir: str) -> bool: print("Restored: {} -> {}".format(backup_file, target_path)) print("Netplan files restored successfully") - return True + return @retry(max_attempts=5, delay=60) @@ -498,7 +499,7 @@ def run(): def main(): - # backup the test plans, because nmcli corrupts them + # backup the netplans, because nmcli corrupts them # and debsums will complain afterwards # This is ugly. Ideally, nmcli should be patched instead temp_dir = tempfile.TemporaryDirectory() @@ -506,11 +507,10 @@ def main(): try: run() - except Exception: - restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) + except: raise - - restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) + finally: + restore_netplan_files(str(temp_dir.name), NETPLAN_DIR) if __name__ == "__main__": diff --git a/providers/base/tests/test_wifi_nmcli_test.py b/providers/base/tests/test_wifi_nmcli_test.py index bcbd9914da..4a58f8fa25 100644 --- a/providers/base/tests/test_wifi_nmcli_test.py +++ b/providers/base/tests/test_wifi_nmcli_test.py @@ -699,8 +699,8 @@ def test_restore_netplan_files_success( [], ] - result = restore_netplan_files(None, str(self.TEST_NETPLAN_DIR.name)) - result = restore_netplan_files( + restore_netplan_files(None, str(self.TEST_NETPLAN_DIR.name)) + restore_netplan_files( str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) @@ -713,11 +713,10 @@ def test_restore_netplan_files_success( ], # Backup files ] - result = restore_netplan_files( + restore_netplan_files( str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) - self.assertTrue(result) mock_remove.assert_called_once_with( str(self.TEST_NETPLAN_DIR.name) + "/old1.yaml" ) @@ -740,7 +739,7 @@ def test_restore_netplan_files_remove_error( ] mock_remove.side_effect = OSError("Permission denied") with self.assertRaises(OSError): - result = restore_netplan_files( + restore_netplan_files( str(self.TEST_BACKUP_DIR.name), str(self.TEST_NETPLAN_DIR.name) ) From 50109e137d3920e20e0506a1691d4ccd0b78d35c Mon Sep 17 00:00:00 2001 From: Antoine Lassagne Date: Wed, 9 Jul 2025 11:11:58 +0200 Subject: [PATCH 7/7] Simplify the trycatch, fix flake8 --- providers/base/bin/wifi_nmcli_test.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/providers/base/bin/wifi_nmcli_test.py b/providers/base/bin/wifi_nmcli_test.py index 8174fbf8b0..84f14e5615 100755 --- a/providers/base/bin/wifi_nmcli_test.py +++ b/providers/base/bin/wifi_nmcli_test.py @@ -507,8 +507,6 @@ def main(): try: run() - except: - raise finally: restore_netplan_files(str(temp_dir.name), NETPLAN_DIR)