From 391fa1526e7cf5e7971e15bbe7f8682725d463a1 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Mon, 9 Dec 2024 18:19:53 -0500 Subject: [PATCH 1/5] Improve tests for ownership and remote_src=true --- test/integration/targets/copy/tasks/tests.yml | 86 +++++++++---------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/test/integration/targets/copy/tasks/tests.yml b/test/integration/targets/copy/tasks/tests.yml index 2b6b5c6ff60..6fc024d7687 100644 --- a/test/integration/targets/copy/tasks/tests.yml +++ b/test/integration/targets/copy/tasks/tests.yml @@ -2135,55 +2135,55 @@ register: testcase5 become: true - - name: gather - Stat the new_dir_with_chown + - name: Stat destination to verify ownership stat: - path: '{{ remote_dir }}/new_dir_with_chown' - register: stat_new_dir_with_chown - - - name: gather - Stat the new_dir_with_chown/file1 - stat: - path: '{{ remote_dir }}/new_dir_with_chown/file1' - register: stat_new_dir_with_chown_file1 + path: "{{ remote_dir }}/{{ item }}" + loop: + - 'new_dir_with_chown' + - 'new_dir_with_chown/file1' + - 'new_dir_with_chown/subdir' + - 'new_dir_with_chown/subdir/file12' + - 'new_dir_with_chown/link_file12' + register: testcase5_stat + + - assert: + that: + - testcase5 is changed + - testcase5_stat.results | map(attribute='stat') | map(attribute='uid') | unique == [ansible_copy_test_user.uid] + - testcase5_stat.results | map(attribute='stat') | map(attribute='gid') | unique == [ansible_copy_test_group.gid] + - testcase5_stat.results | map(attribute='stat') | map(attribute='pw_name') | unique == [ansible_copy_test_user_name] + - testcase5_stat.results | map(attribute='stat') | map(attribute='gr_name') | unique == [ansible_copy_test_user_name] - - name: gather - Stat the new_dir_with_chown/subdir - stat: - path: '{{ remote_dir }}/new_dir_with_chown/subdir' - register: stat_new_dir_with_chown_subdir + - command: whoami + register: who - - name: gather - Stat the new_dir_with_chown/subdir/file12 - stat: - path: '{{ remote_dir }}/new_dir_with_chown/subdir/file12' - register: stat_new_dir_with_chown_subdir_file12 + - name: Modify ownership recursively + copy: + remote_src: True + src: '{{ remote_dir_expanded }}/remote_dir_src/' + dest: '{{ remote_dir_expanded }}/new_dir_with_chown' + owner: '{{ who.stdout }}' + group: '{{ who.stdout }}' + follow: true + register: testcase5_updated + become: true - - name: gather - Stat the new_dir_with_chown/link_file12 + - name: Stat destination to verify updated ownership stat: - path: '{{ remote_dir }}/new_dir_with_chown/link_file12' - register: stat_new_dir_with_chown_link_file12 - - - name: assert - owner and group have changed - assert: + path: "{{ remote_dir }}/{{ item }}" + loop: + - 'new_dir_with_chown' + - 'new_dir_with_chown/file1' + - 'new_dir_with_chown/subdir' + - 'new_dir_with_chown/subdir/file12' + - 'new_dir_with_chown/link_file12' + register: testcase5_updated_stat + + - assert: that: - - testcase5 is changed - - stat_new_dir_with_chown.stat.uid == ansible_copy_test_user.uid - - stat_new_dir_with_chown.stat.gid == ansible_copy_test_group.gid - - stat_new_dir_with_chown.stat.pw_name == ansible_copy_test_user_name - - stat_new_dir_with_chown.stat.gr_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_file1.stat.uid == ansible_copy_test_user.uid - - stat_new_dir_with_chown_file1.stat.gid == ansible_copy_test_group.gid - - stat_new_dir_with_chown_file1.stat.pw_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_file1.stat.gr_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_subdir.stat.uid == ansible_copy_test_user.uid - - stat_new_dir_with_chown_subdir.stat.gid == ansible_copy_test_group.gid - - stat_new_dir_with_chown_subdir.stat.pw_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_subdir.stat.gr_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_subdir_file12.stat.uid == ansible_copy_test_user.uid - - stat_new_dir_with_chown_subdir_file12.stat.gid == ansible_copy_test_group.gid - - stat_new_dir_with_chown_subdir_file12.stat.pw_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_subdir_file12.stat.gr_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_link_file12.stat.uid == ansible_copy_test_user.uid - - stat_new_dir_with_chown_link_file12.stat.gid == ansible_copy_test_group.gid - - stat_new_dir_with_chown_link_file12.stat.pw_name == ansible_copy_test_user_name - - stat_new_dir_with_chown_link_file12.stat.gr_name == ansible_copy_test_user_name + - testcase5_updated is changed + - testcase5_updated_stat.results | map(attribute='stat') | map(attribute='pw_name') | unique == [who.stdout] + - testcase5_updated_stat.results | map(attribute='stat') | map(attribute='gr_name') | unique == [who.stdout] always: - name: execute - remove the user for test From 20bb58b399fb1339a8f3431dbae91305e1b3b668 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Tue, 10 Dec 2024 09:12:05 -0500 Subject: [PATCH 2/5] Check bug now has coverage --- lib/ansible/modules/copy.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index 0e052f76f18..b1500fa71d8 100644 --- a/lib/ansible/modules/copy.py +++ b/lib/ansible/modules/copy.py @@ -456,8 +456,7 @@ def copy_directory(src, dest, module): diff_files_changed = copy_diff_files(src, dest, module) left_only_changed = copy_left_only(src, dest, module) common_dirs_changed = copy_common_dirs(src, dest, module) - owner_group_changed = chown_recursive(dest, module) - changed = any([diff_files_changed, left_only_changed, common_dirs_changed, owner_group_changed]) + changed = any([diff_files_changed, left_only_changed, common_dirs_changed]) return changed From 52765a0afa6294ed1b97f17568eda9e62d832139 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Tue, 10 Dec 2024 09:31:15 -0500 Subject: [PATCH 3/5] Revert bug to check tests pass with current behavior --- lib/ansible/modules/copy.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index b1500fa71d8..0e052f76f18 100644 --- a/lib/ansible/modules/copy.py +++ b/lib/ansible/modules/copy.py @@ -456,7 +456,8 @@ def copy_directory(src, dest, module): diff_files_changed = copy_diff_files(src, dest, module) left_only_changed = copy_left_only(src, dest, module) common_dirs_changed = copy_common_dirs(src, dest, module) - changed = any([diff_files_changed, left_only_changed, common_dirs_changed]) + owner_group_changed = chown_recursive(dest, module) + changed = any([diff_files_changed, left_only_changed, common_dirs_changed, owner_group_changed]) return changed From ef8fbd0fba08fc39d3a8a82b217c67902b6fe200 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Tue, 10 Dec 2024 09:49:31 -0500 Subject: [PATCH 4/5] Fix test for FreeBSD --- test/integration/targets/copy/tasks/tests.yml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/integration/targets/copy/tasks/tests.yml b/test/integration/targets/copy/tasks/tests.yml index 6fc024d7687..8af513553fc 100644 --- a/test/integration/targets/copy/tasks/tests.yml +++ b/test/integration/targets/copy/tasks/tests.yml @@ -2155,15 +2155,18 @@ - testcase5_stat.results | map(attribute='stat') | map(attribute='gr_name') | unique == [ansible_copy_test_user_name] - command: whoami - register: who + register: current_user + + - command: id -gn + register: current_group - name: Modify ownership recursively copy: remote_src: True src: '{{ remote_dir_expanded }}/remote_dir_src/' dest: '{{ remote_dir_expanded }}/new_dir_with_chown' - owner: '{{ who.stdout }}' - group: '{{ who.stdout }}' + owner: '{{ current_user.stdout }}' + group: '{{ current_group.stdout }}' follow: true register: testcase5_updated become: true @@ -2182,8 +2185,8 @@ - assert: that: - testcase5_updated is changed - - testcase5_updated_stat.results | map(attribute='stat') | map(attribute='pw_name') | unique == [who.stdout] - - testcase5_updated_stat.results | map(attribute='stat') | map(attribute='gr_name') | unique == [who.stdout] + - testcase5_updated_stat.results | map(attribute='stat') | map(attribute='pw_name') | unique == [current_user.stdout] + - testcase5_updated_stat.results | map(attribute='stat') | map(attribute='gr_name') | unique == [current_group.stdout] always: - name: execute - remove the user for test From 8dbebbf5e783d1951b7325e8bede35b8180cc345 Mon Sep 17 00:00:00 2001 From: s-hertel <19572925+s-hertel@users.noreply.github.com> Date: Mon, 9 Dec 2024 18:21:09 -0500 Subject: [PATCH 5/5] Remove extra recursion when the destination directory exists --- lib/ansible/modules/copy.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/ansible/modules/copy.py b/lib/ansible/modules/copy.py index 0e052f76f18..6bf7e0dc19f 100644 --- a/lib/ansible/modules/copy.py +++ b/lib/ansible/modules/copy.py @@ -430,19 +430,23 @@ def copy_left_only(src, dest, module): def copy_common_dirs(src, dest, module): changed = False - common_dirs = filecmp.dircmp(src, dest).common_dirs - for item in common_dirs: + dircmp = filecmp.dircmp(src, dest) + for item in dircmp.common_dirs: src_item_path = os.path.join(src, item) dest_item_path = os.path.join(dest, item) b_src_item_path = to_bytes(src_item_path, errors='surrogate_or_strict') b_dest_item_path = to_bytes(dest_item_path, errors='surrogate_or_strict') diff_files_changed = copy_diff_files(b_src_item_path, b_dest_item_path, module) left_only_changed = copy_left_only(b_src_item_path, b_dest_item_path, module) - if diff_files_changed or left_only_changed: + ownership_changed = chown_path(module, b_dest_item_path, module.params['owner'], module.params['group']) + if diff_files_changed or left_only_changed or ownership_changed: changed = True # recurse into subdirectory changed = copy_common_dirs(os.path.join(src, item), os.path.join(dest, item), module) or changed + for item in dircmp.same_files: + dest_item_path = os.path.join(dest, item) + changed |= chown_path(module, dest_item_path, module.params['owner'], module.params['group']) return changed @@ -456,8 +460,7 @@ def copy_directory(src, dest, module): diff_files_changed = copy_diff_files(src, dest, module) left_only_changed = copy_left_only(src, dest, module) common_dirs_changed = copy_common_dirs(src, dest, module) - owner_group_changed = chown_recursive(dest, module) - changed = any([diff_files_changed, left_only_changed, common_dirs_changed, owner_group_changed]) + changed = any([diff_files_changed, left_only_changed, common_dirs_changed]) return changed