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

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).

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.compare_with_original()
-                archive.changed = True
             if archive.remove:
-                try:
+                archive.remove_single_target(path)
-                    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()
-                    )
 
     if archive.destination_exists():
         archive.update_permissions()

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

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 }}"

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"