Add file pruned state

[nicholasmhughes]

What does this PR do?

This PR adds a file.pruned state which can delete empty directories within a given root.

What issues does this PR fix or reference?

Fixes: #62178

Previous Behavior

No native state supported this workflow.

New Behavior

Empty directories can easily be pruned from a tree. Note that the file.rmdir execution module operation was also slightly changed. Errors encountered previously returned the error string. This was changed to a more uniform boolean return. This is unlikely to affect any logic as users were most likely checking for True returns on success and then using a broad else statement, because "falsey" behavior wasn't possible with True vs str returns.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[Ch3LL]

looks like the windows test failures are related to these changes

[OrangeDog]

I feel like this would be better as a recursive argument to file.absent than as a whole separate state.

[nicholasmhughes]

file-absent deletes directories regardless of whether they have contents. I felt that it was too risky for users to leverage a parameter to change that behavior because the results of forgetting to include that option would be catastrophic and result in angry users that deleted something important. Much safer to segment the behavior out into a separate state.

[nicholasmhughes]

re-run pr-debian-10-amd64-py3-pytest

[nicholasmhughes]

re-run pr-ubuntu-2004-amd64-py3-tcp-pytest

[OrangeDog]

Also, rmdir is a verb (action) not an adjective (state).

[nicholasmhughes]

open to suggestions

[nicholasmhughes]

re-run pr-centos-7-x86_64-py3-m2crypto-pytest

[Ch3LL]

looks like there is a merge conflict. Also maybe dir_absent instead of rmdir?

Let me ask the rest of the core team what they think of the naming of the state function here.

[whytewolf]

personally this seems more like a pruned_tree more than absent or rmdir.

[nicholasmhughes]

re-run pr-debian-11-amd64-py3-pytest

[nicholasmhughes]

re-run pr-debian-11-amd64-py3-pytest

[waynew]

Definitely love the idea behind this! @Ch3LL asked for some feedback:

• I don't like rmdir as the state name - sometimes we can't help but to name the states as verbs instead of adjectives, but generally .removed or .absent for states, with .remove or similar action words for the execution modules. I like Thomas' pruned suggestion. It's better than dir_absent which was the only thing I could think of 😂 That also naturally leads to the module being prune and the state pruned, which I like a lot. I guess that would mean that we'd just leave rmdir as is and create a prune module, if we wanted to make that change.
• I have some thoughts about the deleting what we can but then failing. Maybe it doesn't matter and it's OK to partially fail. We could check that at least the folders are empty before attempting to delete all of them - basically do the existing for loop but instead of

os.rmdir(subdir_path)
deleted.append(subdir_path)

do something like

if os.listdir(subdir_path):
    errors.append([subdir_path, "Directory is not empty"])
else:
    to_delete.append(subdir_path)

for dir in to_delete:
    try:
        os.rmdir(dir)
    except OSError as exc:
        ...

But... I don't know if that's super important 🤷 - anyway, just my thoughts - I don't feel super strongly about them (aside from liking adjectives for states and verbs for modules)

[nicholasmhughes]

re-run pr-ubuntu-2204-amd64-py3-pytest

[Ch3LL]

@waynew and @whytewolf can you re-review please

[waynew]

I'm good with this 👍 I did just realize that it might just be worth noting in the documentation that it will take the steamroller approach - i.e. it will prune whatever directories it can, rather than bailing out either at the first error or looking before it leaps.

I don't think it should hold up merging the PR but it might be worth a follow-up PR 🤷