Add support to write_to_textfile for custom tmpdir - #1115
Conversation
Sorry, something went wrong.
While the try/except block does prevent most of the temp files from persisting, if there is a non catchable exception, those temp files continue to pollute the directory. Optionally set the temp directory would let us write to something like /tmp, so the target directory isn't polluted Signed-off-by: Aaditya Dhruv <aadityadhruv@mailbox.org>
|
Tagging @csmarchbanks as per CONTRIBUTING.md guidelines. |
Sorry, something went wrong.
csmarchbanks
left a comment
There was a problem hiding this comment.
Thanks!
I think I would prefer to just not support different filesystems. The atomic operation is important for the expected usage and I would prefer to fail than work around that.
Would you also add a bit to the help text describing what the tmpdir is there for and that it needs to be on the same filesystem?
Sorry, something went wrong.
|
Thanks for the reply. The reason I've ended up adding support for the different filesystems is because /tmp, which is a pretty common location for temp files, is pretty much always on a different filesystem on Linux (tmpfs). If we don't allow different filesystem support, most people wouldn't be able to use /tmp. |
Sorry, something went wrong.
|
That makes sense, but would also be quite the footgun. It would be pretty hard to debug failed or incorrect scrapes in that scenario unless they know about the atomic issues. |
Sorry, something went wrong.
|
@csmarchbanks That makes sense, but this is a completely optional parameter. We can mention a warning in the docustring so that people are aware about it - anyone interested in using this parameter would definitely read the docustring. |
Sorry, something went wrong.
|
@csmarchbanks Actually after thinking about it, I think you are right. We could have a partial write if we are on different filesystems, and that is significantly worse. I'll strip out the shutil stuff. |
Sorry, something went wrong.
|
@csmarchbanks I've made the requested changes. Please let me know if it looks good! |
Sorry, something went wrong.
The tmpdir must be on the same filesystem to ensure an atomic operation takes place. If this is not enforced, there could be partial writes which can lead to partial/incorrect metrics being exported Signed-off-by: Aaditya Dhruv <aadityadhruv@mailbox.org>
csmarchbanks
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
While the try/except block does prevent most of the temp files from persisting, if there is a non catchable exception, those temp files continue to pollute the directory. Optionally set the temp directory would let us write to something like /tmp, so the target directory isn't polluted.
This does mean that the target and source filesystems could be different, hence I've used shutil here. I didn't want to modify the original behavior, specifically for the Windows bit, so I've kept it as is. shutil will use os.rename if the filesystems are the same, so we don't lose that atomic behavior in that case.