ansible/ansible-modules-core — pull request #150 by astorije (2014-10-07 03:35:16+00:00)
Title: Add recursive support for ACL module

This PR enables the support for recursive ACLs. Some refactoring was needed to make this work. It is the new repo structure augmented clone of ansible/ansible#8380.

It fixes ansible/ansible#5053, ansible/ansible#5550 and fix ansible/ansible#7276) as well as a few things along the way:

- PEP8 compliance(-ish)
- Fix erroneous entry param `etype` to `type`
- Param checking messages use RFC 2119 wording)
- Add checks for incompatible states

I did not specify the version in the documentation bit. I will add it when this is merged to have consistent docs.

## Advantages of this PR

- Permissions do not need to be normalized anymore using the `--test` checking system

- `--test` does not check using `getfacl`, so the code is less verbose and less likely to introduce a parsing error (since there is actually - almost - no parsing)

- `--test` makes `--recursive` possible at no performance cost, and checking the `changed` status is neither overly complicated or slow

- `default` can now be used on acl queries as well

- [`X` (`capital-x` / *Special execute*)](https://en.wikipedia.org/wiki/Chmod#Symbolic_modes) is now supported in the permissions with no extra code

## Inconvenients of this PR

- `--test` does not seem to be supported by BSD's `setfacl`. However, this can be solved in the `acl_changed` function only without affecting the rest of the file. Suggestions below.

- `recursive=yes` and `state=query` are currently incompatible because that needs further discussion to ensure backward compatibility. This should be done in another issue/PR though. I will be more than happy to work on it, but one step at a time :)

## General notes (and motivations) to help for the review

- `normalize_permissions` did not support `X` (`capital-x`) and its support would cause problem with the former checking system (since `X` itself is not stateless)

- `_run_acl` was making the usage of `--test` impossible because for the last line returned when using `--test` is relevant and not empty. It now makes sure that the last line is empty before stripping it.

- 2 new builder helpers `build_entry` and `build_command` now factorize some recurring code

- `acl_changed` is a function meant to test if a command changes ACLs. The checking can be extended there rather than in the `main` function

- I removed the `get_acls`, `set_acl` and `rm_acl` wrappers for the following reasons:
- They were used only once each
- They only contained one call to `build_command` and one call to `run_acl` in the end
- The command is already built when it is `--test`ed before so I reuse the command rather than rebuilding it

- `_run_acl` is now `run_acl` because it is not a hidden/private function anymore, it is used in the `main` function multiple times

## Suggestions for Linux / BSD support

My PR breaks support for BSD because of its lack of `--test`, but adds support for `--recursive` on Linux. I suggest that when `--test` is not available, `changed` is **always** true when using `recursive=yes`. When not using `recursive=yes`, we can either do the same or replicate the old code for checking state. What I described earlier (notably the `X` bit) will still be a problem. I'll talk with @bcoca on IRC about that.

The documentation should then mention the support differences if there are any (for example no support for `recursive` on BSD).