Add support to write_to_textfile for custom tmpdir
#1115
Conversation
aad9be9 to
4f72683
Compare
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. |
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?
|
Thanks for the reply. The reason I've ended up adding support for the different filesystems is because |
|
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. |
|
@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. |
|
@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 |
|
@csmarchbanks I've made the requested changes. Please let me know if it looks good! |
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>
While the
try/exceptblock 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
shutilhere. I didn't want to modify the original behavior, specifically for the Windows bit, so I've kept it as is.shutilwill useos.renameif the filesystems are the same, so we don't lose that atomic behavior in that case.