Add atime and mtime support for files ops, start with files.put #1372
Conversation
…pport Add a restricted analog to the ``touch`` command. Supports mutating atime and mtime, however only allows modification of one time field at a time. A single unambiguous datetime val is the time source since any ``touch`` operation with specified atime/mtime requires the month, day, and time at a minimum. Is BSD aware in that datetimes are always converted to UTC rather than relying on the richer timezone support in GNU ``touch``. The single time field setting approach is less flexible per invocation than native ``touch``, but also far simpler and nicely fits with file operations and their way of working on a single attribute at a time.
Time metadate updates are not coalesced, as ``touch`` can do with a single date and time arg, or via reference files. However, this actually allows more flexibility than native ''touch`` by allowing both atime and mtime values to be specified independently. Two support functions are added as well.
Negative timestamps are valid and permissible. Ensure pyinfra.facts.files.stat doesn't fail to match stat command output when such values occur.
….files
In preparation for tests of mtime and ctime support, make the mock os.stat
actually use any stat fields provided in the test json, with non-zero defaults
for all fields.
Doesn't break any existing test, but lays foundation for any future ops or facts
which may use this info.
Doesn't currently support setting the st_{a, m}time_ns high-resolution time
fields.
Change the datetime Python value to be generated from the fromisoformat method rather than strptime. This yields identical results as before when no UTC offset information is provided, but respects it if it does. It is also consistent with the os.stat parsing.
Add several testcases to cover multiple aspects of functionality: - Change detection, incl. differences <1s that are visible in the seconds vals - Separate actions on mtime and atime - Local files as the time references - Remote files as the time references - Use of the server's timezone when a datetime arg has no tzinfo
There was a problem hiding this comment.
Nice, this is really neat @vram0gh2! Sorry it took so long for me to do a proper review. Replying to your notes specifically:
I added a note in the operation docstring, but if atime and mtime support is implemented in other operations (e.g., files.file) these will be true there as well. Is there a preferred way to break that information out into something outside of the function-specific docstring?
Currently not, and yeah that could be messy to duplicate everywhere. One option would be to put it into the top level docstring in operations/files.py such that it appears at the top of the operation doc and then reference that within each operation.
I do think this would be a great to expand to the other ops!
The expanded mock os.stat added a fair number of LOC, but it's also now equipped for future features/higher fidelity.
👍 really nice, good add.
Also, I used the American spelling of "normalize" in one of the added support functions, happy to change it to British if preferred.
All good, American makes sense as consistent w/Python, though I'm sure pyinfra contains a mix of both already 🙃
pyinfra/operations/files.py
Outdated
| @@ -778,6 +778,56 @@ def get( | |||
| yield FileDownloadCommand(src, dest, remote_temp_filename=host.get_temp_filename(dest)) | |||
|
|
|||
|
|
|||
| def _canonicalize_timespec(field, local_file, timespec): | |||
| assert field == "atime" or field == "mtime" | |||
There was a problem hiding this comment.
Using an enum for the field argument would be cleaner here + support type-checking.
pyinfra/operations/files.py
Outdated
| if field == "atime": | ||
| return datetime.fromtimestamp(lf_stat.st_atime, tz=timezone.utc) | ||
| else: | ||
| return datetime.fromtimestamp(lf_stat.st_mtime, tz=timezone.utc) |
There was a problem hiding this comment.
| if field == "atime": | |
| return datetime.fromtimestamp(lf_stat.st_atime, tz=timezone.utc) | |
| else: | |
| return datetime.fromtimestamp(lf_stat.st_mtime, tz=timezone.utc) | |
| tstamp = if_stat.st_atime if field == "atime" else if_stat.st_mtime | |
| return datetime.fromtimestamp(tstamp, tz=timezone.utc) |
Nitpicking, just a bit neater.
pyinfra/operations/files.py
Outdated
| @@ -802,6 +854,9 @@ def put( | |||
| + create_remote_dir: create the remote directory if it doesn't exist | |||
| + force: always upload the file, even if the remote copy matches | |||
| + assume_exists: whether to assume the local file exists | |||
| + atime: value of atime the file should have, use ``True`` to match the local file | |||
| + mtime: value of atime the file should have, use ``True`` to match the local file | |||
| + timesrc: the source of the time value if atime or mtime are ``True`` | |||
In operations/util/files.py, define an enum to indicate whether the metadata time field being manipulated by touch() is atime or mtime.` This feeds into operations/files.py and the _canonicalize_timespec() support function.
|
Thanks for the review, @Fizzadar ! I made changes/fixes as per the feedback . Docstrings should now be fixed, and we're using an There could be some stylistic room to argue in that I gave the enum a somewhat? long name of If this is considered merge-ready otherwise, perhaps we can address the documentation issue if/when |
Add concrete support for
atimeandmtimechecking/setting in thefiles.putoperation and provide infrastructure to add this time metadata support to other operations as well.Includes a fix in
facts/files.pyto accommodate negative timestamps fromstaton the remote host.Added support for more
os.stat_resultfile metadata to be provided in the test json files, by updating the mockos.statpatch used for testing. This includesatime,mtime, but also the other members of the standardstat10-tuple as well.@Fizzadar: Looking for any feedback/implementation concerns, style changes you'd like, and how best to handle documenting the specifics and caveats. I added a note in the operation docstring, but if
atimeandmtimesupport is implemented in other operations (e.g.,files.file) these will be true there as well. Is there a preferred way to break that information out into something outside of the function-specific docstring?The expanded mock
os.statadded a fair number of LOC, but it's also now equipped for future features/higher fidelity.Also, I used the American spelling of "normalize" in one of the added support functions, happy to change it to British if preferred.
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)