Repository: ansible/ansible
Pull request: documentation fix for file module 78988
##### SUMMARY

Fixes #78988 

##### ISSUE TYPE

- Docs Pull Request

##### COMPONENT NAME
file module

lib/ansible/modules/file.py

##### ADDITIONAL INFORMATION
This is for documentation change requested in issue (78988)



Problem statement:
File module changes mode of src when state is link
### Summary

The file module is supposed to only affect `dest:`, but in cases where `state: link` and a `mode:` is supplied, the mode of `src:` — if it exists — is changed.

One could argue whether documenting the current behavior is sufficient vs. whether it's better to "fix" the file module, but it isn't clear what "fix" would mean: (1) ignore `mode:` when `state: link`; (2) fail if `state: link` and a `mode:` is given if (a) `src:` does not exist and/or (b) mode of existing `src:` does not match `mode:`. There are plenty of wrong answers available.

### Issue Type

Documentation Report

### Component Name

file

### Ansible Version

```console
$ ansible --version
ansible [core 2.12.6]
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/utoddl/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/venv-python38-ansible-core-212/lib/python3.8/site-packages/ansible
  ansible collection location = /home/utoddl/.ansible/collections:/usr/share/ansible/collections
  executable location = /opt/venv-python38-ansible-core-212/bin/ansible
  python version = 3.8.12 (default, Sep 16 2021, 10:46:05) [GCC 8.5.0 20210514 (Red Hat 8.5.0-3)]
  jinja version = 3.1.2
  libyaml = True
```


### Configuration

```console
# if using a version older than ansible-core 2.12 you should omit the '-t all'
$ ansible-config dump --only-changed -t all
BECOME_PLUGIN_PATH(env: ANSIBLE_BECOME_PLUGINS) = ['/usr/share/ansible/plugins/become']
CACHE_PLUGIN(/etc/ansible/ansible.cfg) = memory
DEFAULT_ACTION_PLUGIN_PATH(env: ANSIBLE_ACTION_PLUGINS) = ['/usr/share/ansible/plugins/action']
DEFAULT_CACHE_PLUGIN_PATH(env: ANSIBLE_CACHE_PLUGINS) = ['/usr/share/ansible/plugins/cache']
DEFAULT_CALLBACK_PLUGIN_PATH(env: ANSIBLE_CALLBACK_PLUGINS) = ['/usr/share/ansible/plugins/callback']
DEFAULT_CONNECTION_PLUGIN_PATH(env: ANSIBLE_CONNECTION_PLUGINS) = ['/usr/share/ansible/plugins/connection']
DEFAULT_FILTER_PLUGIN_PATH(env: ANSIBLE_FILTER_PLUGINS) = ['/usr/share/ansible/plugins/filter']
DEFAULT_FORKS(/etc/ansible/ansible.cfg) = 5
DEFAULT_GATHERING(env: ANSIBLE_GATHERING) = smart
DEFAULT_GATHER_SUBSET(env: ANSIBLE_GATHER_SUBSET) = ['!all', 'virtual', 'network']
DEFAULT_HOST_LIST(env: ANSIBLE_INVENTORY) = ['…']
DEFAULT_INVENTORY_PLUGIN_PATH(env: ANSIBLE_INVENTORY_PLUGINS) = ['/usr/share/ansible/plugins/inventory']
DEFAULT_LOG_PATH(env: ANSIBLE_LOG_PATH) = /var/log/ansible.log
DEFAULT_LOOKUP_PLUGIN_PATH(env: ANSIBLE_LOOKUP_PLUGINS) = ['/usr/share/ansible/plugins/lookup']
DEFAULT_POLL_INTERVAL(/etc/ansible/ansible.cfg) = 15
DEFAULT_STDOUT_CALLBACK(env: ANSIBLE_STDOUT_CALLBACK) = yaml
DEFAULT_STRATEGY_PLUGIN_PATH(env: ANSIBLE_STRATEGY_PLUGINS) = ['/usr/share/ansible/plugins/strategy']
DEFAULT_TERMINAL_PLUGIN_PATH(env: ANSIBLE_TERMINAL_PLUGINS) = ['/usr/share/ansible/plugins/terminal']
DEFAULT_TEST_PLUGIN_PATH(env: ANSIBLE_TEST_PLUGINS) = ['/usr/share/ansible/plugins/test']
DEFAULT_TIMEOUT(env: ANSIBLE_TIMEOUT) = 30
DEFAULT_TRANSPORT(/etc/ansible/ansible.cfg) = smart
DEFAULT_VARS_PLUGIN_PATH(env: ANSIBLE_VARS_PLUGINS) = ['/usr/share/ansible/plugins/vars']
DEFAULT_VAULT_IDENTITY(env: ANSIBLE_VAULT_IDENTITY) = mw
DEFAULT_VAULT_IDENTITY_LIST(env: ANSIBLE_VAULT_IDENTITY_LIST) = […]
DISPLAY_SKIPPED_HOSTS(env: ANSIBLE_DISPLAY_SKIPPED_HOSTS) = True
HOST_KEY_CHECKING(env: ANSIBLE_HOST_KEY_CHECKING) = False
INTERPRETER_PYTHON(env: ANSIBLE_PYTHON_INTERPRETER) = auto
RETRY_FILES_ENABLED(/etc/ansible/ansible.cfg) = False
TRANSFORM_INVALID_GROUP_CHARS(env: ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS) = silently

BECOME:
======

CACHE:
=====

CALLBACK:
========

default:
_______
display_skipped_hosts(env: ANSIBLE_DISPLAY_SKIPPED_HOSTS) = True

CLICONF:
=======

CONNECTION:
==========

paramiko_ssh:
____________
host_key_checking(env: ANSIBLE_HOST_KEY_CHECKING) = False

ssh:
___
host_key_checking(env: ANSIBLE_HOST_KEY_CHECKING) = False
pipelining(env: ANSIBLE_SSH_PIPELINING) = False
timeout(env: ANSIBLE_TIMEOUT) = 30

HTTPAPI:
=======

INVENTORY:
=========

LOOKUP:
======

NETCONF:
=======

SHELL:
=====

sh:
__
remote_tmp(/etc/ansible/ansible.cfg) = /tmp/${USER}/.ansible/tmp

VARS:
====
```


### OS / Environment

Red Hat Enterprise Linux release 8.6 (Ootpa)

### Additional Information

Without this change, users may reasonably expect symbolic links themselves to have the specified mode — for example 0777 or -rwxrwxrwx — when in fact the target of the link (i.e. `src:`) will become world-writable / clobberable.

### Code of Conduct

- [X] I agree to follow the Ansible Code of Conduct

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/file.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/file.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/devel/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
Effectively the piece you are missing here, is that you want `follow: no`.  Per the documentation:

> follow - This flag indicates that filesystem links, if they exist, should be followed.

This means that the symlink will be created, and then followed when setting the mode.
In the end it always affects `dest`, if that is resolved to `src` (via `follow: yes`) it is still the `dest` that was operated on.
In that case, it's the documentation for `follow:` that's missing a piece, specifically your last sentence. And the synopsis should be changed from "Set attributes of files, symlinks or directories" to "Set attributes of files, symlinks, their targets, or directories."

I had believed, and I think not unreasonably, that `follow:` was irrelevant because it would only be used to affect the behavior of `recurse:`, which itself is only relevant for `state: directory`.

I noticed the phrase "resulting filesystem object" popping up several times. My reading of the docs was that this meant "the file or directory or link given as `path:`", but with this added info it apparently means `src:` if `state: link`.

I understand what you're saying, and it makes sense that it would be implemented that way. But I wouldn't expect the average reader to get the last two comments from reading the current docs. That is not at all obvious, and could be clarified either in the overall description, the docs for `follow:`, or/and the docs for `src:`.
Documentation fix:
 - Change line https://github.com/ansible/ansible/blame/devel/lib/ansible/modules/file.py#L18 to say - 
    Set attributes of files, symlinks, their targets, or directories.

- Add a line below https://github.com/ansible/ansible/blame/devel/lib/ansible/modules/file.py#L74 to say - 
  - If I(follow=yes) and I(state=link),  then I(src) may be impacted.

Commit message: correcting mssing semi-colon

Patch to review:
diff --git a/lib/ansible/modules/file.py b/lib/ansible/modules/file.py
index 55489bcda503cf..0eb8ba0a8a0c6b 100644
--- a/lib/ansible/modules/file.py
+++ b/lib/ansible/modules/file.py
@@ -15,7 +15,7 @@
 short_description: Manage files and file properties
 extends_documentation_fragment: [files, action_common_attributes]
 description:
-- Set attributes of files, symlinks or directories.
+- Set attributes of files, symlinks, their targets, or directories.
 - Alternatively, remove files, symlinks or directories.
 - Many other modules support the same options as the C(file) module - including M(ansible.builtin.copy),
   M(ansible.builtin.template), and M(ansible.builtin.assemble).
@@ -72,6 +72,7 @@
   follow:
     description:
     - This flag indicates that filesystem links, if they exist, should be followed.
+    - If I(follow=yes) and I(state=link), then I(src) may be impacted.
     - Previous to Ansible 2.5, this was C(no) by default.
     type: bool
     default: yes


Code review comments:
diff_hunk: @@ -15,7 +15,7 @@
 short_description: Manage files and file properties
 extends_documentation_fragment: [files, action_common_attributes]
 description:
-- Set attributes of files, symlinks or directories.
+- Set attributes of files, symlinks, their targets, or directories.
path: lib/ansible/modules/file.py
text: @user1:
Does "their targets" refer to the symlink target here? TBH this seems like an unclear pronoun reference to me.

@author:
Yes "their targets" refers to targets  of "symlinks".

@user1:
Thanks for clarifying @author How about `files, directories, or symlinks and their targets`?

@author:
Yes this looks better