From 1e6424138cf7b1d55a12e1710b3d421b081a4a4b Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 15 Jun 2022 08:41:28 -0400 Subject: [PATCH 1/8] add file.rmdir tests for original function --- .../functional/modules/file/test_rmdir.py | 35 +++++++++++++ .../unit/modules/file/test_file_rmdir.py | 50 +++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 tests/pytests/functional/modules/file/test_rmdir.py create mode 100644 tests/pytests/unit/modules/file/test_file_rmdir.py diff --git a/tests/pytests/functional/modules/file/test_rmdir.py b/tests/pytests/functional/modules/file/test_rmdir.py new file mode 100644 index 000000000000..ce8b7458c606 --- /dev/null +++ b/tests/pytests/functional/modules/file/test_rmdir.py @@ -0,0 +1,35 @@ +import os + +import pytest + +pytestmark = [ + pytest.mark.windows_whitelisted, +] + + +@pytest.fixture(scope="module") +def file(modules): + return modules.file + + +@pytest.fixture(scope="function") +def single_empty_dir(tmp_path): + yield str(tmp_path) + + +@pytest.fixture(scope="function") +def single_dir_with_file(tmp_path): + file = tmp_path / "stuff.txt" + file.write_text("things") + yield str(tmp_path) + + +def test_rmdir_success_with_default_options(file, single_empty_dir): + assert file.rmdir(single_empty_dir) is True + assert not os.path.isdir(single_empty_dir) + assert not os.path.exists(single_empty_dir) + + +def test_rmdir_failure_with_default_options(file, single_dir_with_file): + assert file.rmdir(single_dir_with_file) == "Directory not empty" + assert os.path.isdir(single_dir_with_file) diff --git a/tests/pytests/unit/modules/file/test_file_rmdir.py b/tests/pytests/unit/modules/file/test_file_rmdir.py new file mode 100644 index 000000000000..1488f663a339 --- /dev/null +++ b/tests/pytests/unit/modules/file/test_file_rmdir.py @@ -0,0 +1,50 @@ +import logging + +import pytest +import salt.modules.file as filemod +from salt.exceptions import SaltInvocationError +from tests.support.mock import MagicMock, patch + +log = logging.getLogger(__name__) + + +@pytest.fixture +def configure_loader_modules(): + return { + filemod: { + "__salt__": {}, + "__opts__": { + "test": False, + "file_roots": {"base": "tmp"}, + "pillar_roots": {"base": "tmp"}, + "cachedir": "tmp", + "grains": {}, + }, + "__grains__": {}, + "__utils__": {}, + } + } + + +def test_file_rmdir_not_absolute_path_exception(): + with pytest.raises(SaltInvocationError): + filemod.rmdir("not_absolute") + + +def test_file_rmdir_not_found_exception(): + with pytest.raises(SaltInvocationError): + filemod.rmdir("/tmp/not_there") + + +def test_file_rmdir_success_return(): + with patch("os.rmdir", MagicMock(return_value=True)), patch( + "os.path.isdir", MagicMock(return_value=True) + ): + assert filemod.rmdir("/tmp/salt_test_return") is True + + +def test_file_rmdir_failure_return(): + with patch( + "os.rmdir", MagicMock(side_effect=OSError(39, "Directory not empty")) + ), patch("os.path.isdir", MagicMock(return_value=True)): + assert filemod.rmdir("/tmp/salt_test_return") == "Directory not empty" From 8ad5c87c8a2d8a1ecd8c59fb1b20821ce056436c Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 15 Jun 2022 14:57:28 -0400 Subject: [PATCH 2/8] update file.rmdir exec module for recursive operation and verbose output --- salt/modules/file.py | 44 +++++++++- .../functional/modules/file/test_rmdir.py | 87 ++++++++++++++++++- .../unit/modules/file/test_file_rmdir.py | 2 +- 3 files changed, 128 insertions(+), 5 deletions(-) diff --git a/salt/modules/file.py b/salt/modules/file.py index 95bd69a58869..cf45e08ec0ea 100644 --- a/salt/modules/file.py +++ b/salt/modules/file.py @@ -4128,18 +4128,35 @@ def stats(path, hash_type=None, follow_symlinks=True): return ret -def rmdir(path): +def rmdir(path, recurse=False, verbose=False): """ .. versionadded:: 2014.1.0 + .. versionchanged:: 3006.0 + Changed return value for failure to a boolean. Remove the specified directory. Fails if a directory is not empty. + recurse + When ``recurse`` is set to ``True``, all empty directories + within the path are pruned. + + .. versionadded:: 3006.0 + + verbose + When ``verbose`` is set to ``True``, a dictionary is returned + which contains more information about the removal process. + + .. versionadded:: 3006.0 + CLI Example: .. code-block:: bash salt '*' file.rmdir /tmp/foo/ """ + ret = False + deleted = [] + errors = [] path = os.path.expanduser(path) if not os.path.isabs(path): @@ -4148,11 +4165,32 @@ def rmdir(path): if not os.path.isdir(path): raise SaltInvocationError("A valid directory was not specified.") + if recurse: + for root, dirs, _ in os.walk(path, topdown=False): + for subdir in dirs: + subdir_path = os.path.join(root, subdir) + try: + log.debug("Removing '%s'", subdir_path) + os.rmdir(subdir_path) + deleted.append(subdir_path) + except OSError as exc: + errors.append([subdir_path, str(exc)]) + log.error("Could not remove '%s': %s", subdir_path, exc) + ret = not errors + try: os.rmdir(path) - return True + deleted.append(path) + ret = True if ret or not recurse else False except OSError as exc: - return exc.strerror + ret = False + errors.append([path, str(exc)]) + log.error("Could not remove '%s': %s", path, exc) + + if verbose: + return {"deleted": deleted, "errors": errors, "result": ret} + else: + return ret def remove(path): diff --git a/tests/pytests/functional/modules/file/test_rmdir.py b/tests/pytests/functional/modules/file/test_rmdir.py index ce8b7458c606..ef7c5be44b34 100644 --- a/tests/pytests/functional/modules/file/test_rmdir.py +++ b/tests/pytests/functional/modules/file/test_rmdir.py @@ -24,6 +24,35 @@ def single_dir_with_file(tmp_path): yield str(tmp_path) +@pytest.fixture(scope="function") +def nested_empty_dirs(tmp_path): + num_root = 2 + num_mid = 4 + num_last = 2 + for root in range(1, num_root + 1): + for mid in range(1, num_mid + 1): + for last in range(1, num_last + 1): + nest = tmp_path / f"root{root}" / f"mid{mid}" / f"last{last}" + nest.mkdir(parents=True, exist_ok=True) + yield str(tmp_path) + + +@pytest.fixture(scope="function") +def nested_dirs_with_files(tmp_path): + num_root = 2 + num_mid = 4 + num_last = 2 + for root in range(1, num_root + 1): + for mid in range(1, num_mid + 1): + for last in range(1, num_last + 1): + nest = tmp_path / f"root{root}" / f"mid{mid}" / f"last{last}" + nest.mkdir(parents=True, exist_ok=True) + if last % 2: + last_file = nest / "stuff.txt" + last_file.write_text("things") + yield str(tmp_path) + + def test_rmdir_success_with_default_options(file, single_empty_dir): assert file.rmdir(single_empty_dir) is True assert not os.path.isdir(single_empty_dir) @@ -31,5 +60,61 @@ def test_rmdir_success_with_default_options(file, single_empty_dir): def test_rmdir_failure_with_default_options(file, single_dir_with_file): - assert file.rmdir(single_dir_with_file) == "Directory not empty" + assert file.rmdir(single_dir_with_file) is False + assert os.path.isdir(single_dir_with_file) + + +def test_rmdir_single_dir_success_with_recurse(file, single_empty_dir): + assert file.rmdir(single_empty_dir, recurse=True) is True + assert not os.path.isdir(single_empty_dir) + assert not os.path.exists(single_empty_dir) + + +def test_rmdir_single_dir_failure_with_recurse(file, single_dir_with_file): + assert file.rmdir(single_dir_with_file, recurse=True) is False + assert os.path.isdir(single_dir_with_file) + + +def test_rmdir_nested_empty_dirs_failure_with_default_options(file, nested_empty_dirs): + assert file.rmdir(nested_empty_dirs) is False + assert os.path.isdir(nested_empty_dirs) + + +def test_rmdir_nested_empty_dirs_success_with_recurse(file, nested_empty_dirs): + assert file.rmdir(nested_empty_dirs, recurse=True) is True + assert not os.path.isdir(nested_empty_dirs) + assert not os.path.exists(nested_empty_dirs) + + +def test_rmdir_nested_dirs_with_files_failure_with_recurse( + file, nested_dirs_with_files +): + assert file.rmdir(nested_dirs_with_files, recurse=True) is False + assert os.path.isdir(nested_dirs_with_files) + + +def test_rmdir_verbose_nested_dirs_with_files_failure_with_recurse( + file, nested_dirs_with_files +): + ret = file.rmdir(nested_dirs_with_files, recurse=True, verbose=True) + assert ret["result"] is False + assert len(ret["deleted"]) == 8 + assert len(ret["errors"]) == 19 + assert os.path.isdir(nested_dirs_with_files) + + +def test_rmdir_verbose_success(file, single_empty_dir): + ret = file.rmdir(single_empty_dir, verbose=True) + assert ret["result"] is True + assert ret["deleted"][0] == single_empty_dir + assert not ret["errors"] + assert not os.path.isdir(single_empty_dir) + assert not os.path.exists(single_empty_dir) + + +def test_rmdir_verbose_failure(file, single_dir_with_file): + ret = file.rmdir(single_dir_with_file, verbose=True) + assert ret["result"] is False + assert not ret["deleted"] + assert ret["errors"][0][0] == single_dir_with_file assert os.path.isdir(single_dir_with_file) diff --git a/tests/pytests/unit/modules/file/test_file_rmdir.py b/tests/pytests/unit/modules/file/test_file_rmdir.py index 1488f663a339..ccdc943daab2 100644 --- a/tests/pytests/unit/modules/file/test_file_rmdir.py +++ b/tests/pytests/unit/modules/file/test_file_rmdir.py @@ -47,4 +47,4 @@ def test_file_rmdir_failure_return(): with patch( "os.rmdir", MagicMock(side_effect=OSError(39, "Directory not empty")) ), patch("os.path.isdir", MagicMock(return_value=True)): - assert filemod.rmdir("/tmp/salt_test_return") == "Directory not empty" + assert filemod.rmdir("/tmp/salt_test_return") is False From 637265cf42db7f16dc935b623b248cc53de0a5e1 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Thu, 16 Jun 2022 09:38:42 -0400 Subject: [PATCH 3/8] fixes saltstack/salt#62178 add file.rmdir state --- changelog/62178.added | 1 + salt/states/file.py | 65 +++++++++++++++ .../functional/modules/file/test_rmdir.py | 14 +++- .../functional/states/file/test_rmdir.py | 82 +++++++++++++++++++ tests/pytests/unit/states/file/test_rmdir.py | 61 ++++++++++++++ 5 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 changelog/62178.added create mode 100644 tests/pytests/functional/states/file/test_rmdir.py create mode 100644 tests/pytests/unit/states/file/test_rmdir.py diff --git a/changelog/62178.added b/changelog/62178.added new file mode 100644 index 000000000000..52dcdff5efab --- /dev/null +++ b/changelog/62178.added @@ -0,0 +1 @@ +Add rmdir state and expanded rmdir exec module functionality diff --git a/salt/states/file.py b/salt/states/file.py index 9f33a8de232e..e97f27095b94 100644 --- a/salt/states/file.py +++ b/salt/states/file.py @@ -9050,3 +9050,68 @@ def mod_beacon(name, **kwargs): ), "result": False, } + + +def rmdir(name, recurse=False, ignore_errors=False): + """ + .. versionadded:: 3006.0 + + Ensure that the named directory is absent. If it exists and is empty, it + will be deleted. + + name + The directory which should be deleted if empty. + + recurse + If set to ``True``, this option will recursive deletion of empty + directories. This is useful if nested paths are all empty, and would + be the only items preventing removal of the named root directory. + + ignore_errors + If set to ``True``, any errors encountered while attempting to delete a + directory are ignored. This **AUTOMATICALLY ENABLES** the ``recurse`` + option since it's not terribly useful to ignore errors on the removal of + a single directory. Useful for pruning only the empty directories in a + tree which contains non-empty directories as well. + """ + name = os.path.expanduser(name) + + ret = {"name": name, "changes": {}, "comment": "", "result": True} + + if ignore_errors: + recurse = True + + if os.path.isdir(name): + if __opts__["test"]: + ret["result"] = None + ret["changes"]["deleted"] = name + ret["comment"] = "Directory {} is set for removal".format(name) + return ret + + res = __salt__["file.rmdir"](name, recurse=recurse, verbose=True) + result = res.pop("result") + + if result: + if recurse and res["deleted"]: + ret[ + "comment" + ] = "Recursively removed empty directories under {}".format(name) + ret["changes"]["deleted"] = sorted(res["deleted"]) + elif not recurse: + ret["comment"] = "Removed directory {}".format(name) + ret["changes"]["deleted"] = name + return ret + elif ignore_errors and res["deleted"]: + ret["comment"] = "Recursively removed empty directories under {}".format( + name + ) + ret["changes"]["deleted"] = sorted(res["deleted"]) + return ret + + ret["result"] = result + ret["changes"] = res + ret["comment"] = "Failed to remove directory {}".format(name) + return ret + + ret["comment"] = "Directory {} is not present".format(name) + return ret diff --git a/tests/pytests/functional/modules/file/test_rmdir.py b/tests/pytests/functional/modules/file/test_rmdir.py index ef7c5be44b34..d17f1a88f293 100644 --- a/tests/pytests/functional/modules/file/test_rmdir.py +++ b/tests/pytests/functional/modules/file/test_rmdir.py @@ -32,7 +32,12 @@ def nested_empty_dirs(tmp_path): for root in range(1, num_root + 1): for mid in range(1, num_mid + 1): for last in range(1, num_last + 1): - nest = tmp_path / f"root{root}" / f"mid{mid}" / f"last{last}" + nest = ( + tmp_path + / "root{}".format(root) + / "mid{}".format(mid) + / "last{}".format(last) + ) nest.mkdir(parents=True, exist_ok=True) yield str(tmp_path) @@ -45,7 +50,12 @@ def nested_dirs_with_files(tmp_path): for root in range(1, num_root + 1): for mid in range(1, num_mid + 1): for last in range(1, num_last + 1): - nest = tmp_path / f"root{root}" / f"mid{mid}" / f"last{last}" + nest = ( + tmp_path + / "root{}".format(root) + / "mid{}".format(mid) + / "last{}".format(last) + ) nest.mkdir(parents=True, exist_ok=True) if last % 2: last_file = nest / "stuff.txt" diff --git a/tests/pytests/functional/states/file/test_rmdir.py b/tests/pytests/functional/states/file/test_rmdir.py new file mode 100644 index 000000000000..4f0a55683b0b --- /dev/null +++ b/tests/pytests/functional/states/file/test_rmdir.py @@ -0,0 +1,82 @@ +import pytest + +pytestmark = [ + pytest.mark.windows_whitelisted, +] + + +@pytest.fixture(scope="module") +def file(states): + return states.file + + +@pytest.fixture(scope="function") +def single_dir_with_file(tmp_path): + file = tmp_path / "stuff.txt" + file.write_text("things") + yield str(tmp_path) + + +@pytest.fixture(scope="function") +def nested_empty_dirs(tmp_path): + num_root = 2 + num_mid = 4 + num_last = 2 + for root in range(1, num_root + 1): + for mid in range(1, num_mid + 1): + for last in range(1, num_last + 1): + nest = ( + tmp_path + / "root{}".format(root) + / "mid{}".format(mid) + / "last{}".format(last) + ) + nest.mkdir(parents=True, exist_ok=True) + yield str(tmp_path) + + +@pytest.fixture(scope="function") +def nested_dirs_with_files(tmp_path): + num_root = 2 + num_mid = 4 + num_last = 2 + for root in range(1, num_root + 1): + for mid in range(1, num_mid + 1): + for last in range(1, num_last + 1): + nest = ( + tmp_path + / "root{}".format(root) + / "mid{}".format(mid) + / "last{}".format(last) + ) + nest.mkdir(parents=True, exist_ok=True) + if last % 2: + last_file = nest / "stuff.txt" + last_file.write_text("things") + yield str(tmp_path) + + +def test_rmdir_failure(file, single_dir_with_file): + ret = file.rmdir(name=single_dir_with_file) + assert ret.result is False + assert not ret.changes["deleted"] + assert len(ret.changes["errors"]) == 1 + assert ret.comment == "Failed to remove directory {}".format(single_dir_with_file) + + +def test_rmdir_success_recurse_and_deleted(file, nested_empty_dirs): + ret = file.rmdir(name=nested_empty_dirs, recurse=True) + assert ret.result is True + assert len(ret.changes["deleted"]) == 27 + assert ret.comment == "Recursively removed empty directories under {}".format( + nested_empty_dirs + ) + + +def test_rmdir_success_ignore_errors_and_deleted(file, nested_dirs_with_files): + ret = file.rmdir(name=nested_dirs_with_files, ignore_errors=True) + assert ret.result is True + assert len(ret.changes["deleted"]) == 8 + assert ret.comment == "Recursively removed empty directories under {}".format( + nested_dirs_with_files + ) diff --git a/tests/pytests/unit/states/file/test_rmdir.py b/tests/pytests/unit/states/file/test_rmdir.py new file mode 100644 index 000000000000..9150a8a648be --- /dev/null +++ b/tests/pytests/unit/states/file/test_rmdir.py @@ -0,0 +1,61 @@ +import logging +import os + +import pytest +import salt.states.file as filestate +import salt.utils.platform +from tests.support.mock import MagicMock, patch + +log = logging.getLogger(__name__) + + +@pytest.fixture +def configure_loader_modules(): + return {filestate: {"__salt__": {}, "__opts__": {}}} + + +@pytest.fixture +def directory_name(): + name = os.sep + "test" + if salt.utils.platform.is_windows(): + name = "c:" + name + return name + + +def test_rmdir_clean(directory_name): + with patch("os.path.isdir", return_value=False): + ret = filestate.rmdir(name=directory_name) + assert ret == { + "changes": {}, + "comment": "Directory {} is not present".format(directory_name), + "name": directory_name, + "result": True, + } + + +def test_rmdir_test(directory_name): + rmdir = MagicMock(return_value={"result": True}) + with patch("os.path.isdir", return_value=True), patch.dict( + filestate.__opts__, {"test": True} + ): + ret = filestate.rmdir(name=directory_name) + assert ret == { + "changes": {"deleted": directory_name}, + "comment": "Directory {} is set for removal".format(directory_name), + "name": directory_name, + "result": None, + } + + +def test_rmdir_success(directory_name): + rmdir = MagicMock(return_value={"result": True}) + with patch("os.path.isdir", return_value=True), patch.dict( + filestate.__opts__, {"test": False} + ), patch.dict(filestate.__salt__, {"file.rmdir": rmdir}): + ret = filestate.rmdir(name=directory_name) + assert ret == { + "changes": {"deleted": directory_name}, + "comment": "Removed directory {}".format(directory_name), + "name": directory_name, + "result": True, + } From 02033a27950dcc1d9ae953f77879012383b0ebf8 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Fri, 17 Jun 2022 13:14:11 -0400 Subject: [PATCH 4/8] add older_than capability to file.rmdir exec and state modules --- salt/modules/file.py | 54 +++++++++++++------ salt/states/file.py | 12 ++++- .../functional/modules/file/test_rmdir.py | 21 ++++++++ 3 files changed, 69 insertions(+), 18 deletions(-) diff --git a/salt/modules/file.py b/salt/modules/file.py index cf45e08ec0ea..952ad543361c 100644 --- a/salt/modules/file.py +++ b/salt/modules/file.py @@ -4128,7 +4128,7 @@ def stats(path, hash_type=None, follow_symlinks=True): return ret -def rmdir(path, recurse=False, verbose=False): +def rmdir(path, recurse=False, verbose=False, older_than=None): """ .. versionadded:: 2014.1.0 .. versionchanged:: 3006.0 @@ -4148,6 +4148,14 @@ def rmdir(path, recurse=False, verbose=False): .. versionadded:: 3006.0 + older_than + When ``older_than`` is set to a number, it is used to determine the + **number of days** which must have passed since the last modification + timestamp before a directory will be allowed to be removed. Setting + the value to 0 is equivalent to leaving it at the default of ``None``. + + .. versionadded:: 3006.0 + CLI Example: .. code-block:: bash @@ -4165,27 +4173,41 @@ def rmdir(path, recurse=False, verbose=False): if not os.path.isdir(path): raise SaltInvocationError("A valid directory was not specified.") + if older_than: + now = time.time() + try: + older_than = now - (int(older_than) * 86400) + log.debug("Now (%s) looking for directories older than %s", now, older_than) + except (TypeError, ValueError) as exc: + older_than = 0 + log.error("Unable to set 'older_than'. Defaulting to 0 days. (%s)", exc) + if recurse: for root, dirs, _ in os.walk(path, topdown=False): for subdir in dirs: subdir_path = os.path.join(root, subdir) - try: - log.debug("Removing '%s'", subdir_path) - os.rmdir(subdir_path) - deleted.append(subdir_path) - except OSError as exc: - errors.append([subdir_path, str(exc)]) - log.error("Could not remove '%s': %s", subdir_path, exc) + if ( + older_than and os.path.getmtime(subdir_path) < older_than + ) or not older_than: + try: + log.debug("Removing '%s'", subdir_path) + os.rmdir(subdir_path) + deleted.append(subdir_path) + except OSError as exc: + errors.append([subdir_path, str(exc)]) + log.error("Could not remove '%s': %s", subdir_path, exc) ret = not errors - try: - os.rmdir(path) - deleted.append(path) - ret = True if ret or not recurse else False - except OSError as exc: - ret = False - errors.append([path, str(exc)]) - log.error("Could not remove '%s': %s", path, exc) + if (older_than and os.path.getmtime(path) < older_than) or not older_than: + try: + log.debug("Removing '%s'", path) + os.rmdir(path) + deleted.append(path) + ret = True if ret or not recurse else False + except OSError as exc: + ret = False + errors.append([path, str(exc)]) + log.error("Could not remove '%s': %s", path, exc) if verbose: return {"deleted": deleted, "errors": errors, "result": ret} diff --git a/salt/states/file.py b/salt/states/file.py index e97f27095b94..a376fbf23bfb 100644 --- a/salt/states/file.py +++ b/salt/states/file.py @@ -9052,7 +9052,7 @@ def mod_beacon(name, **kwargs): } -def rmdir(name, recurse=False, ignore_errors=False): +def rmdir(name, recurse=False, ignore_errors=False, older_than=None): """ .. versionadded:: 3006.0 @@ -9073,6 +9073,12 @@ def rmdir(name, recurse=False, ignore_errors=False): option since it's not terribly useful to ignore errors on the removal of a single directory. Useful for pruning only the empty directories in a tree which contains non-empty directories as well. + + older_than + When ``older_than`` is set to a number, it is used to determine the + **number of days** which must have passed since the last modification + timestamp before a directory will be allowed to be removed. Setting + the value to 0 is equivalent to leaving it at the default of ``None``. """ name = os.path.expanduser(name) @@ -9088,7 +9094,9 @@ def rmdir(name, recurse=False, ignore_errors=False): ret["comment"] = "Directory {} is set for removal".format(name) return ret - res = __salt__["file.rmdir"](name, recurse=recurse, verbose=True) + res = __salt__["file.rmdir"]( + name, recurse=recurse, verbose=True, older_than=older_than + ) result = res.pop("result") if result: diff --git a/tests/pytests/functional/modules/file/test_rmdir.py b/tests/pytests/functional/modules/file/test_rmdir.py index d17f1a88f293..0dd6bc0476b3 100644 --- a/tests/pytests/functional/modules/file/test_rmdir.py +++ b/tests/pytests/functional/modules/file/test_rmdir.py @@ -1,4 +1,5 @@ import os +import time import pytest @@ -39,6 +40,10 @@ def nested_empty_dirs(tmp_path): / "last{}".format(last) ) nest.mkdir(parents=True, exist_ok=True) + if last % 2: + now = time.time() + old = now - (2 * 86400) + os.utime(str(nest), (old, old)) yield str(tmp_path) @@ -128,3 +133,19 @@ def test_rmdir_verbose_failure(file, single_dir_with_file): assert not ret["deleted"] assert ret["errors"][0][0] == single_dir_with_file assert os.path.isdir(single_dir_with_file) + + +def test_rmdir_nested_empty_dirs_recurse_older_than(file, nested_empty_dirs): + ret = file.rmdir(nested_empty_dirs, recurse=True, verbose=True, older_than=1) + assert ret["result"] is True + assert len(ret["deleted"]) == 8 + assert len(ret["errors"]) == 0 + assert os.path.isdir(nested_empty_dirs) + + +def test_rmdir_nested_empty_dirs_recurse_not_older_than(file, nested_empty_dirs): + ret = file.rmdir(nested_empty_dirs, recurse=True, verbose=True, older_than=3) + assert ret["result"] is True + assert len(ret["deleted"]) == 0 + assert len(ret["errors"]) == 0 + assert os.path.isdir(nested_empty_dirs) From ff8c666e3b5b0919f7b8c8d1a5c2442daf22a372 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 29 Jun 2022 11:28:30 -0400 Subject: [PATCH 5/8] change test to use direct import of file module --- .../functional/modules/file/test_rmdir.py | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/tests/pytests/functional/modules/file/test_rmdir.py b/tests/pytests/functional/modules/file/test_rmdir.py index 0dd6bc0476b3..caad6051bacb 100644 --- a/tests/pytests/functional/modules/file/test_rmdir.py +++ b/tests/pytests/functional/modules/file/test_rmdir.py @@ -2,17 +2,13 @@ import time import pytest +import salt.modules.file as file pytestmark = [ pytest.mark.windows_whitelisted, ] -@pytest.fixture(scope="module") -def file(modules): - return modules.file - - @pytest.fixture(scope="function") def single_empty_dir(tmp_path): yield str(tmp_path) @@ -68,48 +64,46 @@ def nested_dirs_with_files(tmp_path): yield str(tmp_path) -def test_rmdir_success_with_default_options(file, single_empty_dir): +def test_rmdir_success_with_default_options(single_empty_dir): assert file.rmdir(single_empty_dir) is True assert not os.path.isdir(single_empty_dir) assert not os.path.exists(single_empty_dir) -def test_rmdir_failure_with_default_options(file, single_dir_with_file): +def test_rmdir_failure_with_default_options(single_dir_with_file): assert file.rmdir(single_dir_with_file) is False assert os.path.isdir(single_dir_with_file) -def test_rmdir_single_dir_success_with_recurse(file, single_empty_dir): +def test_rmdir_single_dir_success_with_recurse(single_empty_dir): assert file.rmdir(single_empty_dir, recurse=True) is True assert not os.path.isdir(single_empty_dir) assert not os.path.exists(single_empty_dir) -def test_rmdir_single_dir_failure_with_recurse(file, single_dir_with_file): +def test_rmdir_single_dir_failure_with_recurse(single_dir_with_file): assert file.rmdir(single_dir_with_file, recurse=True) is False assert os.path.isdir(single_dir_with_file) -def test_rmdir_nested_empty_dirs_failure_with_default_options(file, nested_empty_dirs): +def test_rmdir_nested_empty_dirs_failure_with_default_options(nested_empty_dirs): assert file.rmdir(nested_empty_dirs) is False assert os.path.isdir(nested_empty_dirs) -def test_rmdir_nested_empty_dirs_success_with_recurse(file, nested_empty_dirs): +def test_rmdir_nested_empty_dirs_success_with_recurse(nested_empty_dirs): assert file.rmdir(nested_empty_dirs, recurse=True) is True assert not os.path.isdir(nested_empty_dirs) assert not os.path.exists(nested_empty_dirs) -def test_rmdir_nested_dirs_with_files_failure_with_recurse( - file, nested_dirs_with_files -): +def test_rmdir_nested_dirs_with_files_failure_with_recurse(nested_dirs_with_files): assert file.rmdir(nested_dirs_with_files, recurse=True) is False assert os.path.isdir(nested_dirs_with_files) def test_rmdir_verbose_nested_dirs_with_files_failure_with_recurse( - file, nested_dirs_with_files + nested_dirs_with_files, ): ret = file.rmdir(nested_dirs_with_files, recurse=True, verbose=True) assert ret["result"] is False @@ -118,7 +112,7 @@ def test_rmdir_verbose_nested_dirs_with_files_failure_with_recurse( assert os.path.isdir(nested_dirs_with_files) -def test_rmdir_verbose_success(file, single_empty_dir): +def test_rmdir_verbose_success(single_empty_dir): ret = file.rmdir(single_empty_dir, verbose=True) assert ret["result"] is True assert ret["deleted"][0] == single_empty_dir @@ -127,7 +121,7 @@ def test_rmdir_verbose_success(file, single_empty_dir): assert not os.path.exists(single_empty_dir) -def test_rmdir_verbose_failure(file, single_dir_with_file): +def test_rmdir_verbose_failure(single_dir_with_file): ret = file.rmdir(single_dir_with_file, verbose=True) assert ret["result"] is False assert not ret["deleted"] @@ -135,7 +129,7 @@ def test_rmdir_verbose_failure(file, single_dir_with_file): assert os.path.isdir(single_dir_with_file) -def test_rmdir_nested_empty_dirs_recurse_older_than(file, nested_empty_dirs): +def test_rmdir_nested_empty_dirs_recurse_older_than(nested_empty_dirs): ret = file.rmdir(nested_empty_dirs, recurse=True, verbose=True, older_than=1) assert ret["result"] is True assert len(ret["deleted"]) == 8 @@ -143,7 +137,7 @@ def test_rmdir_nested_empty_dirs_recurse_older_than(file, nested_empty_dirs): assert os.path.isdir(nested_empty_dirs) -def test_rmdir_nested_empty_dirs_recurse_not_older_than(file, nested_empty_dirs): +def test_rmdir_nested_empty_dirs_recurse_not_older_than(nested_empty_dirs): ret = file.rmdir(nested_empty_dirs, recurse=True, verbose=True, older_than=3) assert ret["result"] is True assert len(ret["deleted"]) == 0 From af4cbaf68913ab57c02077f601deb44fd8b98292 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 29 Jun 2022 13:02:18 -0400 Subject: [PATCH 6/8] Revert "change test to use direct import of file module" This reverts commit ff8c666e3b5b0919f7b8c8d1a5c2442daf22a372. --- .../functional/modules/file/test_rmdir.py | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tests/pytests/functional/modules/file/test_rmdir.py b/tests/pytests/functional/modules/file/test_rmdir.py index caad6051bacb..0dd6bc0476b3 100644 --- a/tests/pytests/functional/modules/file/test_rmdir.py +++ b/tests/pytests/functional/modules/file/test_rmdir.py @@ -2,13 +2,17 @@ import time import pytest -import salt.modules.file as file pytestmark = [ pytest.mark.windows_whitelisted, ] +@pytest.fixture(scope="module") +def file(modules): + return modules.file + + @pytest.fixture(scope="function") def single_empty_dir(tmp_path): yield str(tmp_path) @@ -64,46 +68,48 @@ def nested_dirs_with_files(tmp_path): yield str(tmp_path) -def test_rmdir_success_with_default_options(single_empty_dir): +def test_rmdir_success_with_default_options(file, single_empty_dir): assert file.rmdir(single_empty_dir) is True assert not os.path.isdir(single_empty_dir) assert not os.path.exists(single_empty_dir) -def test_rmdir_failure_with_default_options(single_dir_with_file): +def test_rmdir_failure_with_default_options(file, single_dir_with_file): assert file.rmdir(single_dir_with_file) is False assert os.path.isdir(single_dir_with_file) -def test_rmdir_single_dir_success_with_recurse(single_empty_dir): +def test_rmdir_single_dir_success_with_recurse(file, single_empty_dir): assert file.rmdir(single_empty_dir, recurse=True) is True assert not os.path.isdir(single_empty_dir) assert not os.path.exists(single_empty_dir) -def test_rmdir_single_dir_failure_with_recurse(single_dir_with_file): +def test_rmdir_single_dir_failure_with_recurse(file, single_dir_with_file): assert file.rmdir(single_dir_with_file, recurse=True) is False assert os.path.isdir(single_dir_with_file) -def test_rmdir_nested_empty_dirs_failure_with_default_options(nested_empty_dirs): +def test_rmdir_nested_empty_dirs_failure_with_default_options(file, nested_empty_dirs): assert file.rmdir(nested_empty_dirs) is False assert os.path.isdir(nested_empty_dirs) -def test_rmdir_nested_empty_dirs_success_with_recurse(nested_empty_dirs): +def test_rmdir_nested_empty_dirs_success_with_recurse(file, nested_empty_dirs): assert file.rmdir(nested_empty_dirs, recurse=True) is True assert not os.path.isdir(nested_empty_dirs) assert not os.path.exists(nested_empty_dirs) -def test_rmdir_nested_dirs_with_files_failure_with_recurse(nested_dirs_with_files): +def test_rmdir_nested_dirs_with_files_failure_with_recurse( + file, nested_dirs_with_files +): assert file.rmdir(nested_dirs_with_files, recurse=True) is False assert os.path.isdir(nested_dirs_with_files) def test_rmdir_verbose_nested_dirs_with_files_failure_with_recurse( - nested_dirs_with_files, + file, nested_dirs_with_files ): ret = file.rmdir(nested_dirs_with_files, recurse=True, verbose=True) assert ret["result"] is False @@ -112,7 +118,7 @@ def test_rmdir_verbose_nested_dirs_with_files_failure_with_recurse( assert os.path.isdir(nested_dirs_with_files) -def test_rmdir_verbose_success(single_empty_dir): +def test_rmdir_verbose_success(file, single_empty_dir): ret = file.rmdir(single_empty_dir, verbose=True) assert ret["result"] is True assert ret["deleted"][0] == single_empty_dir @@ -121,7 +127,7 @@ def test_rmdir_verbose_success(single_empty_dir): assert not os.path.exists(single_empty_dir) -def test_rmdir_verbose_failure(single_dir_with_file): +def test_rmdir_verbose_failure(file, single_dir_with_file): ret = file.rmdir(single_dir_with_file, verbose=True) assert ret["result"] is False assert not ret["deleted"] @@ -129,7 +135,7 @@ def test_rmdir_verbose_failure(single_dir_with_file): assert os.path.isdir(single_dir_with_file) -def test_rmdir_nested_empty_dirs_recurse_older_than(nested_empty_dirs): +def test_rmdir_nested_empty_dirs_recurse_older_than(file, nested_empty_dirs): ret = file.rmdir(nested_empty_dirs, recurse=True, verbose=True, older_than=1) assert ret["result"] is True assert len(ret["deleted"]) == 8 @@ -137,7 +143,7 @@ def test_rmdir_nested_empty_dirs_recurse_older_than(nested_empty_dirs): assert os.path.isdir(nested_empty_dirs) -def test_rmdir_nested_empty_dirs_recurse_not_older_than(nested_empty_dirs): +def test_rmdir_nested_empty_dirs_recurse_not_older_than(file, nested_empty_dirs): ret = file.rmdir(nested_empty_dirs, recurse=True, verbose=True, older_than=3) assert ret["result"] is True assert len(ret["deleted"]) == 0 From 82da5520365c050112376a5262eb032df11752bd Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 29 Jun 2022 13:02:48 -0400 Subject: [PATCH 7/8] revert previous test modification and add import to win_file --- salt/modules/win_file.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/modules/win_file.py b/salt/modules/win_file.py index 9b5221c2b7a4..6f26d407bbe5 100644 --- a/salt/modules/win_file.py +++ b/salt/modules/win_file.py @@ -29,6 +29,7 @@ import string # do not remove, used in imported file.py functions import sys # do not remove, used in imported file.py functions import tempfile # do not remove. Used in salt.modules.file.__clean_tmp +import time # do not remove, used in imported file.py functions import urllib.parse from collections.abc import Iterable, Mapping from functools import reduce # do not remove From e81e55d7a46901a968999e3fa8341c49d6001b1e Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 13 Jul 2022 17:38:15 -0400 Subject: [PATCH 8/8] rename file.rmdir state to file.pruned --- changelog/62178.added | 2 +- salt/states/file.py | 5 +++-- .../states/file/{test_rmdir.py => test_pruned.py} | 12 ++++++------ .../states/file/{test_rmdir.py => test_pruned.py} | 13 ++++++------- 4 files changed, 16 insertions(+), 16 deletions(-) rename tests/pytests/functional/states/file/{test_rmdir.py => test_pruned.py} (84%) rename tests/pytests/unit/states/file/{test_rmdir.py => test_pruned.py} (81%) diff --git a/changelog/62178.added b/changelog/62178.added index 52dcdff5efab..6ada29465845 100644 --- a/changelog/62178.added +++ b/changelog/62178.added @@ -1 +1 @@ -Add rmdir state and expanded rmdir exec module functionality +Add file.pruned state and expanded file.rmdir exec module functionality diff --git a/salt/states/file.py b/salt/states/file.py index a376fbf23bfb..e8eb4575adde 100644 --- a/salt/states/file.py +++ b/salt/states/file.py @@ -9052,12 +9052,13 @@ def mod_beacon(name, **kwargs): } -def rmdir(name, recurse=False, ignore_errors=False, older_than=None): +def pruned(name, recurse=False, ignore_errors=False, older_than=None): """ .. versionadded:: 3006.0 Ensure that the named directory is absent. If it exists and is empty, it - will be deleted. + will be deleted. An entire directory tree can be pruned of empty + directories as well, by using the ``recurse`` option. name The directory which should be deleted if empty. diff --git a/tests/pytests/functional/states/file/test_rmdir.py b/tests/pytests/functional/states/file/test_pruned.py similarity index 84% rename from tests/pytests/functional/states/file/test_rmdir.py rename to tests/pytests/functional/states/file/test_pruned.py index 4f0a55683b0b..101fa76d2cc6 100644 --- a/tests/pytests/functional/states/file/test_rmdir.py +++ b/tests/pytests/functional/states/file/test_pruned.py @@ -56,16 +56,16 @@ def nested_dirs_with_files(tmp_path): yield str(tmp_path) -def test_rmdir_failure(file, single_dir_with_file): - ret = file.rmdir(name=single_dir_with_file) +def test_pruned_failure(file, single_dir_with_file): + ret = file.pruned(name=single_dir_with_file) assert ret.result is False assert not ret.changes["deleted"] assert len(ret.changes["errors"]) == 1 assert ret.comment == "Failed to remove directory {}".format(single_dir_with_file) -def test_rmdir_success_recurse_and_deleted(file, nested_empty_dirs): - ret = file.rmdir(name=nested_empty_dirs, recurse=True) +def test_pruned_success_recurse_and_deleted(file, nested_empty_dirs): + ret = file.pruned(name=nested_empty_dirs, recurse=True) assert ret.result is True assert len(ret.changes["deleted"]) == 27 assert ret.comment == "Recursively removed empty directories under {}".format( @@ -73,8 +73,8 @@ def test_rmdir_success_recurse_and_deleted(file, nested_empty_dirs): ) -def test_rmdir_success_ignore_errors_and_deleted(file, nested_dirs_with_files): - ret = file.rmdir(name=nested_dirs_with_files, ignore_errors=True) +def test_pruned_success_ignore_errors_and_deleted(file, nested_dirs_with_files): + ret = file.pruned(name=nested_dirs_with_files, ignore_errors=True) assert ret.result is True assert len(ret.changes["deleted"]) == 8 assert ret.comment == "Recursively removed empty directories under {}".format( diff --git a/tests/pytests/unit/states/file/test_rmdir.py b/tests/pytests/unit/states/file/test_pruned.py similarity index 81% rename from tests/pytests/unit/states/file/test_rmdir.py rename to tests/pytests/unit/states/file/test_pruned.py index 9150a8a648be..26d88d62b4cb 100644 --- a/tests/pytests/unit/states/file/test_rmdir.py +++ b/tests/pytests/unit/states/file/test_pruned.py @@ -22,9 +22,9 @@ def directory_name(): return name -def test_rmdir_clean(directory_name): +def test_pruned_clean(directory_name): with patch("os.path.isdir", return_value=False): - ret = filestate.rmdir(name=directory_name) + ret = filestate.pruned(name=directory_name) assert ret == { "changes": {}, "comment": "Directory {} is not present".format(directory_name), @@ -33,12 +33,11 @@ def test_rmdir_clean(directory_name): } -def test_rmdir_test(directory_name): - rmdir = MagicMock(return_value={"result": True}) +def test_pruned_test(directory_name): with patch("os.path.isdir", return_value=True), patch.dict( filestate.__opts__, {"test": True} ): - ret = filestate.rmdir(name=directory_name) + ret = filestate.pruned(name=directory_name) assert ret == { "changes": {"deleted": directory_name}, "comment": "Directory {} is set for removal".format(directory_name), @@ -47,12 +46,12 @@ def test_rmdir_test(directory_name): } -def test_rmdir_success(directory_name): +def test_pruned_success(directory_name): rmdir = MagicMock(return_value={"result": True}) with patch("os.path.isdir", return_value=True), patch.dict( filestate.__opts__, {"test": False} ), patch.dict(filestate.__salt__, {"file.rmdir": rmdir}): - ret = filestate.rmdir(name=directory_name) + ret = filestate.pruned(name=directory_name) assert ret == { "changes": {"deleted": directory_name}, "comment": "Removed directory {}".format(directory_name),