Repository: ansible/ansible
Pull request: file: fix setting relative paths for src for hard links
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes #55971
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
file


Problem statement:
file state=hard fails with relative path, docs say it should pass
##### SUMMARY

The docs say that the file module accepts a relative path for `src`. But if you try that with `state=hard` it fails.

##### ISSUE TYPE

- Bug Report

##### COMPONENT NAME

file module

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```
ansible 2.9.0.dev0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/opentelco/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/opentelco/patches/ansible/lib/ansible
  executable location = /home/opentelco/patches/ansible/bin/ansible
  python version = 2.7.15rc1 (default, Nov 12 2018, 14:31:15) [GCC 7.3.0]

```

##### CONFIGURATION
`ansible-config dump --only-changed`: empty

##### OS / ENVIRONMENT

Ubuntu 18


##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---
- hosts: localhost
  tasks:
     - name: create hard link
       file:
          dest: hard.txt
          src: file.txt
          state: hard
```

##### EXPECTED RESULTS

File `hard.txt` should be a hard link to `file.txt`.


##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```
TASK [create hard link] ***************************************************************************************************************
fatal: [localhost]: FAILED! => {"changed": false, "msg": "src must be an absolute path"}

```

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

If these files are inaccurate, 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/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
I have verified this. There is a comment related to this from @abadger in https://github.com/ansible/ansible/blob/affaedf/lib/ansible/modules/files/file.py#L760.
My comment in the code might make this apparent but you have a huge +1 from
me to change the behavior of that code.  Might need a porting guide entry
if there are some non error cases that change with the change

On Fri, May 3, 2019, 7:38 AM Martin Krizek <notifications@github.com> wrote:

> I have verified this. There is a comment related to this from @abadger
> <https://github.com/abadger> in
> https://github.com/ansible/ansible/blob/affaedf/lib/ansible/modules/files/file.py#L760
> .
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ansible/ansible/issues/55971#issuecomment-489067203>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABTCWSVO3FKQUS2NRQN3UTPTQP3TANCNFSM4HJRZ57A>
> .
>

Commit message: Add relpath test

Patch to review:
diff --git a/changelogs/fragments/55971-file-relative-path-hard-link.yaml b/changelogs/fragments/55971-file-relative-path-hard-link.yaml
new file mode 100644
index 00000000000000..0d06c14c34c028
--- /dev/null
+++ b/changelogs/fragments/55971-file-relative-path-hard-link.yaml
@@ -0,0 +1,2 @@
+bugfixes:
+  - file - Fix setting relative paths for hard links (https://github.com/ansible/ansible/issues/55971)
diff --git a/lib/ansible/modules/files/file.py b/lib/ansible/modules/files/file.py
index 63d6abd4662a03..62480e8f6e67aa 100644
--- a/lib/ansible/modules/files/file.py
+++ b/lib/ansible/modules/files/file.py
@@ -52,8 +52,7 @@
     description:
     - Path of the file to link to.
     - This applies only to C(state=link) and C(state=hard).
-    - Will accept absolute and non-existing paths.
-    - Will accept relative paths unless state=hard.
+    - For C(state=link), this will also accept a non-existing path.
     - Relative paths are relative to the file being created (C(path)) which is how
       the Unix command C(ln -s SRC DEST) treats relative paths.
     type: path
@@ -753,35 +752,13 @@ def ensure_hardlink(path, src, follow, force, timestamps):
     mtime = get_timestamp_for_time(timestamps['modification_time'], timestamps['modification_time_format'])
     atime = get_timestamp_for_time(timestamps['access_time'], timestamps['access_time_format'])
 
-    # src is the source of a hardlink.  We require it if we are creating a new hardlink
-    if src is None and not os.path.exists(b_path):
-        raise AnsibleModuleError(results={'msg': 'src and dest are required for creating new hardlinks'})
-
-    # Toshio: Begin suspect block
-    # I believe that this block of code is wrong for hardlinks.
-    # src may be relative.
-    # If it is relative, it should be relative to the cwd (so just use abspath).
-    # This is different from symlinks where src is relative to the symlink's path.
-
-    # Why must src be an absolute path?
-    if not os.path.isabs(b_src):
-        raise AnsibleModuleError(results={'msg': "src must be an absolute path"})
-
-    # If this is a link, then it can't be a dir so why is it in the conditional?
-    if not os.path.islink(b_path) and os.path.isdir(b_path):
-        relpath = path
-    else:
-        b_relpath = os.path.dirname(b_path)
-        relpath = to_native(b_relpath, errors='strict')
+    # src is the source of a hardlink.  We require it if we are creating a new hardlink.
+    # We require path in the argument_spec so we know it is present at this point.
+    if src is None:
+        raise AnsibleModuleError(results={'msg': 'src is required for creating new hardlinks'})
 
-    # Why? This does nothing because src was checked to be absolute above?
-    absrc = os.path.join(relpath, src)
-    b_absrc = to_bytes(absrc, errors='surrogate_or_strict')
-    if not force and not os.path.exists(b_absrc):
-        raise AnsibleModuleError(results={'msg': 'src file does not exist, use "force=yes" if you'
-                                                 ' really want to create the link: %s' % absrc,
-                                          'path': path, 'src': src})
-    # Toshio: end suspect block
+    if not os.path.exists(b_src):
+        raise AnsibleModuleError(results={'msg': 'src does not exist', 'dest': path, 'src': src})
 
     diff = initial_diff(path, 'hard', prev_state)
     changed = False
diff --git a/test/integration/targets/file/tasks/main.yml b/test/integration/targets/file/tasks/main.yml
index 52957fff054593..35a71bab3a8337 100644
--- a/test/integration/targets/file/tasks/main.yml
+++ b/test/integration/targets/file/tasks/main.yml
@@ -563,9 +563,6 @@
   stat: path={{output_dir}}/test_follow_rec_target_dir/foo
   register: file_in_dir_result
 
-- debug: var=file_result.stat.mode
-- debug: var=dir_result.stat.mode
-- debug: var=file_in_dir_result.stat.mode
 - name: assert that the link targets were unmodified
   assert:
     that:
@@ -588,12 +585,77 @@
   stat: path={{output_dir}}/test_follow_rec_target_dir/foo
   register: file_in_dir_result
 
-- debug: var=file_result.stat.mode
-- debug: var=dir_result.stat.mode
-- debug: var=file_in_dir_result.stat.mode
 - name: assert that the link targets were modified
   assert:
     that:
     - file_result.stat.mode == '0600'
     - dir_result.stat.mode == '0600'
     - file_in_dir_result.stat.mode == '0600'
+
+# https://github.com/ansible/ansible/issues/55971
+- name: Test missing src and path
+  file:
+    state: hard
+  register: file_error1
+  ignore_errors: yes
+
+- assert:
+    that:
+      - "file_error1 is failed"
+      - "file_error1.msg == 'missing required arguments: path'"
+
+- name: Test missing src
+  file:
+    dest: "{{ output_dir }}/hard.txt"
+    state: hard
+  register: file_error2
+  ignore_errors: yes
+
+- assert:
+    that:
+      - "file_error2 is failed"
+      - "file_error2.msg == 'src is required for creating new hardlinks'"
+
+- name: Test non-existing src
+  file:
+    src: non-existing-file-that-does-not-exist.txt
+    dest: "{{ output_dir }}/hard.txt"
+    state: hard
+  register: file_error3
+  ignore_errors: yes
+
+- assert:
+    that:
+      - "file_error3 is failed"
+      - "file_error3.msg == 'src does not exist'"
+      - "file_error3.dest == '{{ output_dir }}/hard.txt' | expanduser"
+      - "file_error3.src == 'non-existing-file-that-does-not-exist.txt'"
+
+- block:
+  - name: Create a testing file
+    file:
+      dest: original_file.txt
+      state: touch
+
+  - name: Test relative path with state=hard
+    file:
+      src: original_file.txt
+      dest: hard_link_file.txt"
+      state: hard
+    register: hard_link_relpath
+
+  - name: Just check if it was successful, we don't care about the actual hard link in this test
+    assert:
+      that:
+        - "hard_link_relpath is success"
+
+  always:
+    - name: Clean up
+      file:
+        path: "{{ item }}"
+        state: absent
+      loop:
+        - original_file.txt
+        - hard_link_file.txt
+
+# END #55971


Code review comments:
diff_hunk: @@ -588,12 +585,77 @@
   stat: path={{output_dir}}/test_follow_rec_target_dir/foo
   register: file_in_dir_result
 
-- debug: var=file_result.stat.mode
-- debug: var=dir_result.stat.mode
-- debug: var=file_in_dir_result.stat.mode
 - name: assert that the link targets were modified
   assert:
     that:
     - file_result.stat.mode == '0600'
     - dir_result.stat.mode == '0600'
     - file_in_dir_result.stat.mode == '0600'
+
+# https://github.com/ansible/ansible/issues/55971
+- name: Test missing src and path
+  file:
+    state: hard
+  register: file_error1
+  ignore_errors: yes
+
+- assert:
+    that:
+      - "file_error1 is failed"
+      - "file_error1.msg == 'missing required arguments: path'"
+
+- name: Test missing src
+  file:
+    dest: "{{ output_dir }}/hard.txt"
+    state: hard
+  register: file_error2
+  ignore_errors: yes
+
+- assert:
+    that:
+      - "file_error2 is failed"
+      - "file_error2.msg == 'src is required for creating new hardlinks'"
+
+- name: Test non-existing src
+  file:
+    src: non-existing-file-that-does-not-exist.txt
+    dest: "{{ output_dir }}/hard.txt"
+    state: hard
+  register: file_error3
+  ignore_errors: yes
+
+- assert:
+    that:
+      - "file_error3 is failed"
+      - "file_error3.msg == 'src does not exist'"
+      - "file_error3.dest == '{{ output_dir }}/hard.txt' | expanduser"
+      - "file_error3.src == 'non-existing-file-that-does-not-exist.txt'"
+
+- block:
+  - name: Create a testing file
+    file:
+      dest: original_file.txt
+      state: touch
+
+  - name: Test relative path with state=hard
+    file:
+      src: original_file.txt
+      dest: hard_link_file.txt"
path: test/integration/targets/file/tasks/main.yml
text: @user1:
Is that a dangling quote?
```suggestion
      dest: hard_link_file.txt
```

@author:
Good catch, thanks.