From e208fe59329a45966d23f28bd92c0ee5592ac71b Mon Sep 17 00:00:00 2001 From: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Date: Fri, 7 Oct 2022 14:24:46 -0400 Subject: [PATCH] copy module - fix copying directories recursively with remote_src=True (#76997) * copy module - fix copying directories containing modified subdirs with remote_src=True. Previously, the first changed subdir would prevent recursively checking for changes for in subdirs at the same level. * Fix reporting changed for copying empty directories with remote_src=True. If a directory is created on the remote but nothing else, changed is True. --- ...76997-fix-copy-subdirs-with-remote-src.yml | 4 + lib/ansible/modules/copy.py | 8 +- test/integration/targets/copy/tasks/tests.yml | 152 ++++++++++++++++++ 3 files changed, 159 insertions(+), 5 deletions(-) create mode 100644 changelogs/fragments/76997-fix-copy-subdirs-with-remote-src.yml diff --git a/changelogs/fragments/76997-fix-copy-subdirs-with-remote-src.yml b/changelogs/fragments/76997-fix-copy-subdirs-with-remote-src.yml new file mode 100644 index 00000000000..938ab2f7b2c --- /dev/null +++ b/changelogs/fragments/76997-fix-copy-subdirs-with-remote-src.yml @@ -0,0 +1,4 @@ +bugfixes: + - copy remote_src=true - fix copying subdirs recursively when the dest exists and the src and dest have + multiple common subdirectories in a common directory (https://github.com/ansible/ansible/issues/74536). + - copy remote_src=true - fix reporting changed for copying empty directories. diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index 7fed4a5c8f1..fad9ba3a844 100644 --- a/lib/ansible/modules/copy.py +++ b/lib/ansible/modules/copy.py @@ -518,7 +518,7 @@ def copy_common_dirs(src, dest, module): changed = True # recurse into subdirectory - changed = changed or copy_common_dirs(os.path.join(src, item), os.path.join(dest, item), module) + changed = copy_common_dirs(os.path.join(src, item), os.path.join(dest, item), module) or changed return changed @@ -616,6 +616,7 @@ def main(): module.fail_json(**e.results) os.makedirs(b_dirname) + changed = True directory_args = module.load_file_common_arguments(module.params) directory_mode = module.params["directory_mode"] if directory_mode is not None: @@ -744,8 +745,6 @@ def main(): except (IOError, OSError): module.fail_json(msg="failed to copy: %s to %s" % (src, dest), traceback=traceback.format_exc()) changed = True - else: - changed = False # If neither have checksums, both src and dest are directories. if checksum_src is None and checksum_dest is None: @@ -793,13 +792,12 @@ def main(): b_dest = to_bytes(os.path.join(b_dest, b_basename), errors='surrogate_or_strict') if not module.check_mode and not os.path.exists(b_dest): os.makedirs(b_dest) + changed = True b_src = to_bytes(os.path.join(module.params['src'], ""), errors='surrogate_or_strict') diff_files_changed = copy_diff_files(b_src, b_dest, module) left_only_changed = copy_left_only(b_src, b_dest, module) common_dirs_changed = copy_common_dirs(b_src, b_dest, module) owner_group_changed = chown_recursive(b_dest, module) - if diff_files_changed or left_only_changed or common_dirs_changed or owner_group_changed: - changed = True if module.check_mode and not os.path.exists(b_dest): changed = True diff --git a/test/integration/targets/copy/tasks/tests.yml b/test/integration/targets/copy/tasks/tests.yml index 72203563320..b808ce4f8e8 100644 --- a/test/integration/targets/copy/tasks/tests.yml +++ b/test/integration/targets/copy/tasks/tests.yml @@ -419,6 +419,80 @@ - "stat_results2.stat.checksum == ('foo.txt\n'|hash('sha1'))" - "stat_results2.stat.mode == '0547'" +# +# test copying an empty dir to a dest dir with remote_src=True +# + +- name: create empty test dir + file: + path: '{{ remote_dir }}/testcase_empty_dir' + state: directory + +- name: test copying an empty dir to a dir that does not exist (dest ends with slash) + copy: + src: '{{ remote_dir }}/testcase_empty_dir/' + remote_src: yes + dest: '{{ remote_dir }}/testcase_empty_dir_dest/' + register: copy_result + +- name: get stat of newly created dir + stat: + path: '{{ remote_dir }}/testcase_empty_dir_dest' + register: stat_result + +- assert: + that: + - copy_result.changed + - stat_result.stat.exists + - stat_result.stat.isdir + +- name: test no change is made running the task twice + copy: + src: '{{ remote_dir }}/testcase_empty_dir/' + remote_src: yes + dest: '{{ remote_dir }}/testcase_empty_dir_dest/' + register: copy_result + failed_when: copy_result is changed + +- name: remove to test dest with no trailing slash + file: + path: '{{ remote_dir }}/testcase_empty_dir_dest/' + state: absent + +- name: test copying an empty dir to a dir that does not exist (both src/dest have no trailing slash) + copy: + src: '{{ remote_dir }}/testcase_empty_dir' + remote_src: yes + dest: '{{ remote_dir }}/testcase_empty_dir_dest' + register: copy_result + +- name: get stat of newly created dir + stat: + path: '{{ remote_dir }}/testcase_empty_dir_dest' + register: stat_result + +- assert: + that: + - copy_result.changed + - stat_result.stat.exists + - stat_result.stat.isdir + +- name: test no change is made running the task twice + copy: + src: '{{ remote_dir }}/testcase_empty_dir/' + remote_src: yes + dest: '{{ remote_dir }}/testcase_empty_dir_dest/' + register: copy_result + failed_when: copy_result is changed + +- name: clean up src and dest + file: + path: "{{ item }}" + state: absent + loop: + - '{{ remote_dir }}/testcase_empty_dir' + - '{{ remote_dir }}/testcase_empty_dir_dest' + # # test recursive copy local_follow=False, no trailing slash # @@ -2284,3 +2358,81 @@ that: - fail_copy_directory_with_enc_file is failed - fail_copy_directory_with_enc_file.msg == 'A vault password or secret must be specified to decrypt {{role_path}}/files-different/vault/vault-file' + +# +# Test for issue 74536: recursively copy all nested directories with remote_src=yes and src='dir/' when dest exists +# +- vars: + src: '{{ remote_dir }}/testcase_74536' + block: + - name: create source dir with 3 nested subdirs + file: + path: '{{ src }}/a/b1/c1' + state: directory + + - name: copy the source dir with a trailing slash + copy: + src: '{{ src }}/' + remote_src: yes + dest: '{{ src }}_dest/' + register: copy_result + failed_when: copy_result is not changed + + - name: remove the source dir to recreate with different subdirs + file: + path: '{{ src }}' + state: absent + + - name: recreate source dir + file: + path: "{{ item }}" + state: directory + loop: + - '{{ src }}/a/b1/c2' + - '{{ src }}/a/b2/c3' + + - name: copy the source dir containing new subdirs into the existing dest dir + copy: + src: '{{ src }}/' + remote_src: yes + dest: '{{ src }}_dest/' + register: copy_result + + - name: stat each directory that should exist + stat: + path: '{{ item }}' + register: stat_result + loop: + - '{{ src }}_dest' + - '{{ src }}_dest/a' + - '{{ src }}_dest/a/b1' + - '{{ src }}_dest/a/b2' + - '{{ src }}_dest/a/b1/c1' + - '{{ src }}_dest/a/b1/c2' + - '{{ src }}_dest/a/b2/c3' + + - debug: msg="{{ stat_result }}" + + - assert: + that: + - copy_result is changed + # all paths exist + - stat_result.results | map(attribute='stat') | map(attribute='exists') | unique == [true] + # all paths are dirs + - stat_result.results | map(attribute='stat') | map(attribute='isdir') | unique == [true] + + - name: copy the src again to verify no changes will be made + copy: + src: '{{ src }}/' + remote_src: yes + dest: '{{ src }}_dest/' + register: copy_result + failed_when: copy_result is changed + + - name: clean up src and dest + file: + path: '{{ item }}' + state: absent + loop: + - '{{ src }}' + - '{{ src }}_dest'