Files op idempotency (#630)

* Implement idempotency for the `files.download` operation.

* Add/remove idempotency reasoning on files link/directory/file operations.

* Implement idempotency for the `files.put` operations.

This also adds idempotency for the `files.sync`, `files.template`,
`server.script`, `server.script_template` operations and also when
adding manually defined repositories in `yum.repo`, `dnf.repo` and
`zypper.repo`.

* Implement idempotency for the `files.line` & `files.replace` operations.

* Add non idempotent reasons to server script operations.

* Cleanup files fact docstrings.

* Fix/add missing default value for `files.Find*` facts.

* Fix file upload op API test.

* Mock out slow paramiko `LazyFqdn` object in SSH client tests.

* Use `del [:]` as replacement for missing list `clear` method in Python 2.

Author: Fizzadar

pyinfra/facts/files.py

@@ -131,7 +131,8 @@ class Socket(File):
 
 class Sha1File(FactBase):
     '''
-    Returns a SHA1 hash of a file. Works with both sha1sum and sha1.
+    Returns a SHA1 hash of a file. Works with both sha1sum and sha1. Returns
+    ``None`` if the file doest not exist.
     '''
 
     _regexes = [
@@ -157,7 +158,7 @@ def process(self, output):
 
 class Sha256File(FactBase):
     '''
-    Returns a SHA256 hash of a file.
+    Returns a SHA256 hash of a file, or ``None`` if the file does not exist.
     '''
 
     _regexes = [
@@ -185,7 +186,7 @@ def process(self, output):
 
 class Md5File(FactBase):
     '''
-    Returns an MD5 hash of a file.
+    Returns an MD5 hash of a file, or ``None`` if the file does not exist.
     '''
 
     _regexes = [
@@ -232,9 +233,18 @@ def process(self, output):
         return output
 
 
-class FindFiles(FactBase):
+class FindFilesBase(FactBase):
+    abstract = True
+    default = list
+
+    @staticmethod
+    def process(output):
+        return output
+
+
+class FindFiles(FindFilesBase):
     '''
-    Returns a list of files from a start point, recursively using find.
+    Returns a list of files from a start path, recursively using find.
     '''
 
     @staticmethod
@@ -244,14 +254,10 @@ def command(path):
             QuoteString(path),
         )
 
-    @staticmethod
-    def process(output):
-        return output
-
 
-class FindLinks(FindFiles):
+class FindLinks(FindFilesBase):
     '''
-    Returns a list of links from a start point, recursively using find.
+    Returns a list of links from a start path, recursively using find.
     '''
 
     @staticmethod
@@ -262,9 +268,9 @@ def command(path):
         )
 
 
-class FindDirectories(FindFiles):
+class FindDirectories(FindFilesBase):
     '''
-    Returns a list of directories from a start point, recursively using find.
+    Returns a list of directories from a start path, recursively using find.
     '''
 
     @staticmethod

pyinfra/operations/files.py

@@ -67,7 +67,7 @@ def download(
     state=None, host=None,
 ):
     '''
-    Download files from remote locations using curl or wget.
+    Download files from remote locations using ``curl`` or ``wget``.
 
     + src: source URL of the file
     + dest: where to save the file
@@ -92,6 +92,8 @@ def download(
     '''
 
     info = host.get_fact(File, path=dest)
+    host_datetime = host.get_fact(Date).replace(tzinfo=None)
+
     # Destination is a directory?
     if info is False:
         raise OperationError(
@@ -111,8 +113,8 @@ def download(
         if cache_time:
             # Time on files is not tz-aware, and will be the same tz as the server's time,
             # so we can safely remove the tzinfo from the Date fact before comparison.
-            cache_time = host.get_fact(Date).replace(tzinfo=None) - timedelta(seconds=cache_time)
-            if info['mtime'] and info['mtime'] > cache_time:
+            cache_time = host_datetime - timedelta(seconds=cache_time)
+            if info['mtime'] and info['mtime'] < cache_time:
                 download = True
 
         if sha1sum:
@@ -171,6 +173,24 @@ def download(
                 '|| ( echo {2} && exit 1 )'
             ), QuoteString(dest), md5sum, QuoteString('MD5 did not match!'))
 
+        host.create_fact(
+            File,
+            kwargs={'path': dest},
+            data={'mode': mode, 'group': group, 'user': user, 'mtime': host_datetime},
+        )
+
+        # Remove any checksum facts as we don't know the correct values
+        for value, fact_cls in (
+            (sha1sum, Sha1File),
+            (sha256sum, Sha256File),
+            (md5sum, Md5File),
+        ):
+            fact_kwargs = {'path': dest}
+            if value:
+                host.create_fact(fact_cls, kwargs=fact_kwargs, data=value)
+            else:
+                host.delete_fact(fact_cls, kwargs=fact_kwargs)
+
     else:
         host.noop('file {0} has already been downloaded'.format(dest))
 
@@ -332,6 +352,12 @@ def line(
             else:
                 host.noop('line "{0}" exists in {1}'.format(replace or line, path))
 
+        host.create_fact(
+            FindInFile,
+            kwargs={'path': path, 'pattern': match_line},
+            data=[replace or line],
+        )
+
     # Line(s) exists and we want to remove them, replace with nothing
     elif present_lines and not present:
         yield sed_replace(
@@ -341,11 +367,18 @@ def line(
             interpolate_variables=interpolate_variables,
         )
 
+        host.delete_fact(
+            FindInFile,
+            kwargs={'path': path, 'pattern': match_line},
+        )
+
     # Line(s) exists and we have want to ensure they're correct
     elif present_lines and present:
         # If any of lines are different, sed replace them
         if replace and any(line != replace for line in present_lines):
             yield sed_replace_command
+            del present_lines[:]  # TODO: use .clear() when py3+
+            present_lines.append(replace)
         else:
             host.noop('line "{0}" exists in {1}'.format(replace or line, path))
 
@@ -395,6 +428,11 @@ def replace(
             backup=backup,
             interpolate_variables=interpolate_variables,
         )
+        host.create_fact(
+            FindInFile,
+            kwargs={'path': path, 'pattern': match},
+            data=[],
+        )
     else:
         host.noop('string "{0}" does not exist in {1}'.format(match, path))
 
@@ -578,10 +616,15 @@ def _create_remote_dir(state, host, remote_filename, user, group):
         )
 
 
-@operation(pipeline_facts={
-    'file': 'src',
-    'sha1_file': 'src',
-})
+@operation(
+    # We don't (currently) cache the local state, so there's nothing we can
+    # update to flag the local file as present.
+    is_idempotent=False,
+    pipeline_facts={
+        'file': 'src',
+        'sha1_file': 'src',
+    },
+)
 def get(
     src, dest,
     add_deploy_dir=True, create_local_dir=False, force=False,
@@ -705,6 +748,8 @@ def put(
     if create_remote_dir:
         yield _create_remote_dir(state, host, dest, user, group)
 
+    local_sum = get_file_sha1(src)
+
     # No remote file, always upload and user/group/mode if supplied
     if not remote_file or force:
         yield FileUploadCommand(local_file, dest)
@@ -717,7 +762,6 @@ def put(
 
     # File exists, check sum and check user/group/mode if supplied
     else:
-        local_sum = get_file_sha1(src)
         remote_sum = host.get_fact(Sha1File, path=dest)
 
         # Check sha1sum, upload if needed
@@ -749,6 +793,14 @@ def put(
             if not changed:
                 host.noop('file {0} is already uploaded'.format(dest))
 
+    # Now we've uploaded the file and ensured user/group/mode, update the relevant fact data
+    host.create_fact(Sha1File, kwargs={'path': dest}, data=local_sum)
+    host.create_fact(
+        File,
+        kwargs={'path': dest},
+        data={'user': user, 'group': group, 'mode': mode},
+    )
+
 
 @operation
 def template(

tests/operations/apt.deb/add.json

@@ -1,6 +1,7 @@
 {
     "args": ["http://somewhere.com/something.deb"],
     "facts": {
+        "server.Date": "datetime:2015-01-01T00:00:00",
         "files.File": {
             "path=_tempfile_": ""
         },

tests/operations/apt.deb/add_existing.json

@@ -1,6 +1,7 @@
 {
     "args": ["http://somewhere.com/something.deb"],
     "facts": {
+        "server.Date": "datetime:2015-01-01T00:00:00",
         "files.File": {
             "path=_tempfile_": ""
         },

tests/operations/apt.deb/download_add.json

@@ -1,6 +1,7 @@
 {
     "args": ["http://somewhere.com/something.deb"],
     "facts": {
+        "server.Date": "datetime:2015-01-01T00:00:00",
         "files.File": {
             "path=_tempfile_": null
         },

tests/operations/apt.deb/remove_existing.json

@@ -4,6 +4,7 @@
         "present": false
     },
     "facts": {
+        "server.Date": "datetime:2015-01-01T00:00:00",
         "files.File": {
             "path=_tempfile_": ""
         },

tests/operations/apt.deb/remove_no_exist.json

@@ -4,6 +4,7 @@
         "present": false
     },
     "facts": {
+        "server.Date": "datetime:2015-01-01T00:00:00",
         "files.File": {
             "path=_tempfile_": ""
         },

tests/operations/bsdinit.service/disabled.json

@@ -16,6 +16,5 @@
     },
     "commands": [
         "sed -i.a-timestamp 's/^nginx_enable=.*$//' /etc/rc.conf.local && rm -f /etc/rc.conf.local.a-timestamp"
-    ],
-    "idempotent": false
+    ]
 }

tests/operations/bsdinit.service/enabled.json

@@ -15,6 +15,5 @@
     },
     "commands": [
         "echo 'nginx_enable=\"YES\"' >> /etc/rc.conf.local"
-    ],
-    "idempotent": false
+    ]
 }

tests/operations/dnf.repo/add.json

@@ -18,6 +18,5 @@
         "upload",
         "[somerepo]\nname=description\nbaseurl=http://baseurl\nenabled=1\ngpgcheck=0\ngpgkey=test\n",
             "/etc/yum.repos.d/somerepo.repo"
-    ]],
-    "idempotent": false
+    ]]
 }