Fix copy to only follow symlinks for files in the non-recursive case

Revert "**Temporary**"

This reverts commit 28b86b1148.

We don't need this now that copy has been fixed

changelogs/fragments/copy-files-default-follow.yaml

@@ -0,0 +1,4 @@
+---
+bugfixes:
+- copy - fixed copy to only follow symlinks for files in the non-recursive case
+- file - fixed the default follow behaviour of file to be true

lib/ansible/modules/files/file.py

@@ -182,7 +182,7 @@ def main():
             original_basename=dict(required=False),  # Internal use only, for recursive ops
             recurse=dict(default=False, type='bool'),
             force=dict(required=False, default=False, type='bool'),
-            follow=dict(required=False, default=False, type='bool'),
+            follow=dict(required=False, default=True, type='bool'),
             diff_peek=dict(default=None),  # Internal use only, for internal checks in the action plugins
             validate=dict(required=False, default=None),  # Internal use only, for template and copy
             src=dict(required=False, default=None, type='path'),

lib/ansible/plugins/action/copy.py

@@ -192,9 +192,9 @@ class ActionModule(ActionBase):
         # remove action plugin only keys
         return dict((k, v) for k, v in module_args.items() if k not in ('content', 'decrypt'))
 
-    def _copy_file(self, source_full, source_rel, content, content_tempfile, dest, task_vars):
+    def _copy_file(self, source_full, source_rel, content, content_tempfile,
+                   dest, task_vars, follow):
         decrypt = boolean(self._task.args.get('decrypt', True), strict=False)
-        follow = boolean(self._task.args.get('follow', False), strict=False)
         force = boolean(self._task.args.get('force', 'yes'), strict=False)
         raw = boolean(self._task.args.get('raw', 'no'), strict=False)
 
@@ -289,6 +289,7 @@ class ActionModule(ActionBase):
                     src=tmp_src,
                     dest=dest,
                     original_basename=source_rel,
+                    follow=follow
                 )
             )
             if not self._task.args.get('checksum'):
@@ -491,7 +492,14 @@ class ActionModule(ActionBase):
         for source_full, source_rel in source_files['files']:
             # copy files over.  This happens first as directories that have
             # a file do not need to be created later
-            module_return = self._copy_file(source_full, source_rel, content, content_tempfile, dest, task_vars)
+
+            # We only follow symlinks for files in the non-recursive case
+            if source_files['directories']:
+                follow = False
+            else:
+                follow = boolean(self._task.args.get('follow', False), strict=False)
+
+            module_return = self._copy_file(source_full, source_rel, content, content_tempfile, dest, task_vars, follow)
             if module_return is None:
                 continue
 
@@ -528,6 +536,9 @@ class ActionModule(ActionBase):
             new_module_args['src'] = target_path
             new_module_args['state'] = 'link'
             new_module_args['force'] = True
+            # Only follow remote symlinks in the non-recursive case
+            if source_files['directories']:
+                new_module_args['follow'] = False
 
             module_return = self._execute_module(module_name='file', module_args=new_module_args, task_vars=task_vars)
             module_executed = True

test/integration/targets/copy/tasks/main.yml

@@ -14,7 +14,7 @@
       name: ensure temp dir exists
 
     # file cannot do this properly, use command instead
-    - name: Create ciruclar symbolic link
+    - name: Create circular symbolic link
       command: ln -s ../ circles
       args:
         chdir: '{{role_path}}/files/subdir/subdir1'
@@ -57,7 +57,7 @@
 
     - name: Remove circular symbolic link
       file:
-        path: subdir/subdir1/circles
+        path: '{{ role_path }}/files/subdir/subdir1/circles'
         state: absent
       connection: local
 

test/integration/targets/copy/tasks/tests.yml

@@ -16,6 +16,10 @@
     mode: 0444
   register: copy_result
 
+- name: Record the sha of the test file for later tests
+  set_fact:
+    remote_file_hash: "{{ copy_result['checksum'] }}"
+
 - name: Check the mode of the output file
   file:
     name: "{{ remote_file }}"
@@ -173,7 +177,7 @@
 - name: Create a hardlink to the file
   file:
     src: '{{ remote_file }}'
-    dest: '{{ output_dir }}/hard.lnk'
+    dest: '{{ remote_dir }}/hard.lnk'
     state: hard
 
 - name: copy the same contents into place
@@ -191,7 +195,7 @@
 
 - name: Check the stat results of the hard link
   stat:
-    path: "{{ output_dir }}/hard.lnk"
+    path: "{{ remote_dir }}/hard.lnk"
   register: hlink_results
 
 - name: Check that the file did not change
@@ -216,7 +220,7 @@
 
 - name: Check the stat results of the hard link
   stat:
-    path: "{{ output_dir }}/hard.lnk"
+    path: "{{ remote_dir }}/hard.lnk"
   register: hlink_results
 
 - name: Check that the file changed permissions but is still the same
@@ -242,7 +246,7 @@
 
 - name: Check the stat results of the hard link
   stat:
-    path: "{{ output_dir }}/hard.lnk"
+    path: "{{ remote_dir }}/hard.lnk"
   register: hlink_results
 
 - name: Check that the file changed and hardlink was broken
@@ -1105,6 +1109,10 @@
 
   remote_user: root
 
+#
+# Follow=True tests
+#
+
 # test overwriting a link using "follow=yes" so that the link
 # is preserved and the link target is updated
 
@@ -1122,7 +1130,7 @@
 - name: Update the test file using follow=True to preserve the link
   copy:
     dest: "{{ remote_dir }}/follow_link"
-    content: "this is the new content\n"
+    src: foo.txt
     follow: yes
   register: replace_follow_result
 
@@ -1131,19 +1139,37 @@
     path: "{{ remote_dir }}/follow_link"
   register: stat_link_result
 
-- name: Assert that the link is still a link
+- name: Assert that the link is still a link and contents were changed
   assert:
     that:
-    - stat_link_result.stat.islnk
+    - stat_link_result['stat']['islnk']
+    - stat_link_result['stat']['lnk_target'] == './follow_test'
+    - replace_follow_result['changed']
+    - "replace_follow_result['checksum'] == remote_file_hash"
 
-- name: Get the checksum of the link target
-  shell: "{{ ansible_python_interpreter }} -c 'import hashlib; print(hashlib.sha1(open(\"{{remote_dir | expanduser}}/follow_test\", \"rb\").read()).hexdigest())'"
-  register: target_file_result
+# Symlink handling when the dest is already there
+# https://github.com/ansible/ansible-modules-core/issues/1568
 
-- name: Assert that the link target was updated
+- name: test idempotency by trying to copy to the symlink with the same contents
+  copy:
+    dest: "{{ remote_dir }}/follow_link"
+    src: foo.txt
+    follow: yes
+  register: replace_follow_result
+
+- name: Stat the link path
+  stat:
+    path: "{{ remote_dir }}/follow_link"
+  register: stat_link_result
+
+- name: Assert that the link is still a link and contents were changed
   assert:
     that:
-    - replace_follow_result.checksum == target_file_result.stdout
+    - stat_link_result['stat']['islnk']
+    - stat_link_result['stat']['lnk_target'] == './follow_test'
+    - not replace_follow_result['changed']
+    - replace_follow_result['checksum'] == remote_file_hash
+
 
 - name: Update the test file using follow=False to overwrite the link
   copy:
@@ -1169,6 +1195,13 @@
        - "stat_results.stat.checksum == ('modified'|hash('sha1'))"
        - "not stat_results.stat.islnk"
 
+# test overwriting a link using "follow=yes" so that the link
+# is preserved and the link target is updated when the thing being copied is a link
+
+#
+# File mode tests
+#
+
 - name: setup directory for test
   file: state=directory dest={{remote_dir }}/directory mode=0755