From 9fd2ba60df27b3ae2b194f4e0372c438957be572 Mon Sep 17 00:00:00 2001 From: Ajpantuso Date: Mon, 19 Jul 2021 02:14:23 -0400 Subject: [PATCH] archive - staging idempotency fix (#2987) * Initial Commit * Fixing PY26 filter * Adding changelog fragment * Removing checksum related code * Removing list comparisons due to Jinja errors * Applying review suggestions * Applying review suggestions - typos --- .../2987-archive-stage-idempotency-fix.yml | 4 + plugins/modules/files/archive.py | 44 +++--- .../targets/archive/tasks/main.yml | 7 + .../targets/archive/tests/core.yml | 2 +- .../targets/archive/tests/idempotency.yml | 141 ++++++++++++++++++ 5 files changed, 179 insertions(+), 19 deletions(-) create mode 100644 changelogs/fragments/2987-archive-stage-idempotency-fix.yml create mode 100644 tests/integration/targets/archive/tests/idempotency.yml diff --git a/changelogs/fragments/2987-archive-stage-idempotency-fix.yml b/changelogs/fragments/2987-archive-stage-idempotency-fix.yml new file mode 100644 index 0000000000..5c9e980935 --- /dev/null +++ b/changelogs/fragments/2987-archive-stage-idempotency-fix.yml @@ -0,0 +1,4 @@ +--- +minor_changes: + - archive - refactoring prior to fix for idempotency checks. The fix will be a breaking change and only appear + in community.general 4.0.0 (https://github.com/ansible-collections/community.general/pull/2987). diff --git a/plugins/modules/files/archive.py b/plugins/modules/files/archive.py index 822ea1cd9d..91a8f688f5 100644 --- a/plugins/modules/files/archive.py +++ b/plugins/modules/files/archive.py @@ -298,6 +298,8 @@ class Archive(object): msg='Error, must specify "dest" when archiving multiple files or trees' ) + self.original_size = self.destination_size() + def add(self, path, archive_name): try: self._add(_to_native_ascii(path), _to_native(archive_name)) @@ -315,7 +317,7 @@ class Archive(object): self.destination_state = STATE_ARCHIVED else: try: - f_out = self._open_compressed_file(_to_native_ascii(self.destination)) + f_out = self._open_compressed_file(_to_native_ascii(self.destination), 'wb') with open(path, 'rb') as f_in: shutil.copyfileobj(f_in, f_out) f_out.close() @@ -368,9 +370,15 @@ class Archive(object): msg='Errors when writing archive at %s: %s' % (_to_native(self.destination), '; '.join(self.errors)) ) + def compare_with_original(self): + self.changed |= self.original_size != self.destination_size() + def destination_exists(self): return self.destination and os.path.exists(self.destination) + def destination_readable(self): + return self.destination and os.access(self.destination, os.R_OK) + def destination_size(self): return os.path.getsize(self.destination) if self.destination_exists() else 0 @@ -407,6 +415,15 @@ class Archive(object): def has_unfound_targets(self): return bool(self.not_found) + def remove_single_target(self, path): + try: + os.remove(path) + except OSError as e: + self.module.fail_json( + path=_to_native(path), + msg='Unable to remove source file: %s' % _to_native(e), exception=format_exc() + ) + def remove_targets(self): for path in self.successes: if os.path.exists(path): @@ -453,14 +470,14 @@ class Archive(object): 'expanded_exclude_paths': [_to_native(p) for p in self.expanded_exclude_paths], } - def _open_compressed_file(self, path): + def _open_compressed_file(self, path, mode): f = None if self.format == 'gz': - f = gzip.open(path, 'wb') + f = gzip.open(path, mode) elif self.format == 'bz2': - f = bz2.BZ2File(path, 'wb') + f = bz2.BZ2File(path, mode) elif self.format == 'xz': - f = lzma.LZMAFile(path, 'wb') + f = lzma.LZMAFile(path, mode) else: self.module.fail_json(msg="%s is not a valid format" % self.format) @@ -542,7 +559,7 @@ class TarArchive(Archive): return None if matches_exclusion_patterns(tarinfo.name, self.exclusion_patterns) else tarinfo def py26_filter(path): - return matches_exclusion_patterns(path, self.exclusion_patterns) + return legacy_filter(path, self.exclusion_patterns) if PY27: self.file.add(path, archive_name, recursive=False, filter=py27_filter) @@ -580,7 +597,6 @@ def main(): check_mode = module.check_mode archive = get_archive(module) - size = archive.destination_size() archive.find_targets() if not archive.has_targets(): @@ -592,10 +608,9 @@ def main(): else: archive.add_targets() archive.destination_state = STATE_INCOMPLETE if archive.has_unfound_targets() else STATE_ARCHIVED + archive.compare_with_original() if archive.remove: archive.remove_targets() - if archive.destination_size() != size: - archive.changed = True else: if check_mode: if not archive.destination_exists(): @@ -603,16 +618,9 @@ def main(): else: path = archive.paths[0] archive.add_single_target(path) - if archive.destination_size() != size: - archive.changed = True + archive.compare_with_original() if archive.remove: - try: - os.remove(path) - except OSError as e: - module.fail_json( - path=_to_native(path), - msg='Unable to remove source file: %s' % _to_native(e), exception=format_exc() - ) + archive.remove_single_target(path) if archive.destination_exists(): archive.update_permissions() diff --git a/tests/integration/targets/archive/tasks/main.yml b/tests/integration/targets/archive/tasks/main.yml index e0757b0ead..1e2c9f9c27 100644 --- a/tests/integration/targets/archive/tasks/main.yml +++ b/tests/integration/targets/archive/tasks/main.yml @@ -121,6 +121,13 @@ loop_control: loop_var: format +- name: Run Idempotency tests + include_tasks: + file: ../tests/idempotency.yml + loop: "{{ formats }}" + loop_control: + loop_var: format + # Test cleanup - name: Remove backports.lzma if previously installed (pip) pip: name=backports.lzma state=absent diff --git a/tests/integration/targets/archive/tests/core.yml b/tests/integration/targets/archive/tests/core.yml index f12e5083cc..d008e9c122 100644 --- a/tests/integration/targets/archive/tests/core.yml +++ b/tests/integration/targets/archive/tests/core.yml @@ -41,7 +41,7 @@ - archive_no_options is changed - "archive_no_options.dest_state == 'archive'" - "{{ archive_no_options.archived | length }} == 3" - - + - name: Remove the archive - no options ({{ format }}) file: path: "{{ output_dir }}/archive_no_options.{{ format }}" diff --git a/tests/integration/targets/archive/tests/idempotency.yml b/tests/integration/targets/archive/tests/idempotency.yml new file mode 100644 index 0000000000..f53f768164 --- /dev/null +++ b/tests/integration/targets/archive/tests/idempotency.yml @@ -0,0 +1,141 @@ +--- +- name: Archive - file content idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/*.txt" + dest: "{{ output_dir }}/archive_file_content_idempotency.{{ format }}" + format: "{{ format }}" + register: file_content_idempotency_before + +- name: Modify file - file content idempotency ({{ format }}) + lineinfile: + line: bar.txt + regexp: "^foo.txt$" + path: "{{ output_dir }}/foo.txt" + +- name: Archive second time - file content idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/*.txt" + dest: "{{ output_dir }}/archive_file_content_idempotency.{{ format }}" + format: "{{ format }}" + register: file_content_idempotency_after + +# After idempotency fix result will be reliably changed for all formats +- name: Assert task status is changed - file content idempotency ({{ format }}) + assert: + that: + - file_content_idempotency_after is not changed + when: "format in ('tar', 'zip')" + +- name: Remove archive - file content idempotency ({{ format }}) + file: + path: "{{ output_dir }}/archive_file_content_idempotency.{{ format }}" + state: absent + +- name: Modify file back - file content idempotency ({{ format }}) + lineinfile: + line: foo.txt + regexp: "^bar.txt$" + path: "{{ output_dir }}/foo.txt" + +- name: Archive - file name idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/*.txt" + dest: "{{ output_dir }}/archive_file_name_idempotency.{{ format }}" + format: "{{ format }}" + register: file_name_idempotency_before + +- name: Rename file - file name idempotency ({{ format }}) + command: "mv {{ output_dir}}/foo.txt {{ output_dir }}/fii.txt" + +- name: Archive again - file name idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/*.txt" + dest: "{{ output_dir }}/archive_file_name_idempotency.{{ format }}" + format: "{{ format }}" + register: file_name_idempotency_after + +# After idempotency fix result will be reliably changed for all formats +- name: Check task status - file name idempotency ({{ format }}) + assert: + that: + - file_name_idempotency_after is not changed + when: "format in ('tar', 'zip')" + +- name: Remove archive - file name idempotency ({{ format }}) + file: + path: "{{ output_dir }}/archive_file_name_idempotency.{{ format }}" + state: absent + +- name: Rename file back - file name idempotency ({{ format }}) + command: "mv {{ output_dir }}/fii.txt {{ output_dir }}/foo.txt" + +- name: Archive - single file content idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/foo.txt" + dest: "{{ output_dir }}/archive_single_file_content_idempotency.{{ format }}" + format: "{{ format }}" + register: single_file_content_idempotency_before + +- name: Modify file - single file content idempotency ({{ format }}) + lineinfile: + line: bar.txt + regexp: "^foo.txt$" + path: "{{ output_dir }}/foo.txt" + +- name: Archive second time - single file content idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/foo.txt" + dest: "{{ output_dir }}/archive_single_file_content_idempotency.{{ format }}" + format: "{{ format }}" + register: single_file_content_idempotency_after + +# After idempotency fix result will be reliably changed for all formats +- name: Assert task status is changed - single file content idempotency ({{ format }}) + assert: + that: + - single_file_content_idempotency_after is not changed + when: "format in ('tar', 'zip')" + +- name: Remove archive - single file content idempotency ({{ format }}) + file: + path: "{{ output_dir }}/archive_single_file_content_idempotency.{{ format }}" + state: absent + +- name: Modify file back - single file content idempotency ({{ format }}) + lineinfile: + line: foo.txt + regexp: "^bar.txt$" + path: "{{ output_dir }}/foo.txt" + +- name: Archive - single file name idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/foo.txt" + dest: "{{ output_dir }}/archive_single_file_name_idempotency.{{ format }}" + format: "{{ format }}" + register: single_file_name_idempotency_before + +- name: Rename file - single file name idempotency ({{ format }}) + command: "mv {{ output_dir}}/foo.txt {{ output_dir }}/fii.txt" + +- name: Archive again - single file name idempotency ({{ format }}) + archive: + path: "{{ output_dir }}/fii.txt" + dest: "{{ output_dir }}/archive_single_file_name_idempotency.{{ format }}" + format: "{{ format }}" + register: single_file_name_idempotency_after + + +# After idempotency fix result will be reliably changed for all formats +- name: Check task status - single file name idempotency ({{ format }}) + assert: + that: + - single_file_name_idempotency_after is not changed + when: "format in ('tar', 'zip')" + +- name: Remove archive - single file name idempotency ({{ format }}) + file: + path: "{{ output_dir }}/archive_single_file_name_idempotency.{{ format }}" + state: absent + +- name: Rename file back - single file name idempotency ({{ format }}) + command: "mv {{ output_dir }}/fii.txt {{ output_dir }}/foo.txt"