Use application tmp dir for product import #12649
Conversation
The file path was never going to be 'tmp/product_import', so I guess it never deleted the file. Hmm but shouldn't we clean up on success too? I suppose the tmp dir will be cleaned up eventually, and maybe we want to keep them for debugging purposes.
In openfoodfoundation#3435, it was switched to the system tmp dir because it conventiently provided a method to generate a unique filename. However it didn't handle the case where the system provided a symlink (macOS). I could have fixed that, but surely it's safer to use the Rails tmp directory. So I changed back to that, using a tip from https://stackoverflow.com/questions/13787746/creating-a-thread-safe-temporary-file-name to generate a unique name. Perhaps we could use a larger string (eg uuid) or append a timestamp too, but I don't know that it's necessary. Instead, we can just check that the dir didn't exist first (as mentioned in the PR). Let's do that..
This method is not documented, but is used by Dir.mktmpdir. https://apidock.com/ruby/Dir/Tmpname/create
dacook
added
the
technical changes only
| def tmpdir_base | ||
| Rails.root.join('tmp', TMPDIR_PREFIX).to_s | ||
| end |
There was a problem hiding this comment.
This could be a constant which can then be re-used in specs.
There was a problem hiding this comment.
The knowledge of the path construction is in multiple places now as well. Instead of having a directory name prefix, we could just create a directory tmp/product_import/ and then have random filenames within. Would that make more sense?
There was a problem hiding this comment.
Yeah that would probably make more sense, but we would need to re-implement without Dir::Tmpname.create (which handles name collisions for us).
Hmm, I guess that's what make_tmpname mentioned in #3435 is for, but that method was abandoned on that PR. I forget why, but as you say I don't think we should put more energy in ;)
|
Hi @dacook, I have tested the following:
ConclusionIt's looking good! 💪 Thanks again, David! |
|
Thanks Konrad! |
⚠ Please use clockify code #7198 Back Office Uplift (Product) while reviewing or working on this task.
What? Why?
This came up because the product import doesn't work on my macOS. So I fixed it in order to continue with:
What should we test?