Implement scm package for handling ignored files#50
Implement scm package for handling ignored files#50onyxraven merged 14 commits intoIbotta:masterfrom randrusiak:feature/add-file-to-scm-ignores
Conversation
onyxraven
left a comment
There was a problem hiding this comment.
This looks great so far! This is definitely one of those that has been brought up internally.
Like the other PR, I'm going to review this a bit more and ensure this gets through testing.
Thanks!
onyxraven
left a comment
There was a problem hiding this comment.
Comments here are mostly about avoiding loading the ignore file into memory. The chances that the file is big enough to worry about is near-nothing though, so I'm OK if that work is deferred.
|
Thank you for your feedback. As you noticed, the gitignore file is usually small, but I think it's always a good idea to write better code. |
|
Sorry for the delay. I made suggested fixes, and now it works without loading unnecessary data into memory :) |
onyxraven
left a comment
There was a problem hiding this comment.
couple small notes, otherwise looks good.
| } | ||
|
|
||
| // Override target file with tempFile | ||
| err = os.Rename(tempFile.Name(), filename) |
There was a problem hiding this comment.
I can't remember, do you need to fsync or close the tempFile before you rename it here? It might be fine.
There was a problem hiding this comment.
Currently, it works so maybe it's handled internally but I'm not sure how to do it properly.
There was a problem hiding this comment.
I can call file.Sync() before renaming. But what about closing a file?
I think I can close the file exactly before renaming (without deferring) or defer the anonymous function, which closes a file and rename it.
Which approach do you think is better?
There was a problem hiding this comment.
I'll do a little looking around, but I think this should be OK for now.
I believe in linux type OSs this should be fine, because the file handle and the filename are not the same, so there should be no problem, so I think this is probably fine for now.
There was a problem hiding this comment.
reading through recommendations, this may be safe on unix-like fd systems (you can rename a file that your user has open) but may have some undefined behavior, especially if there are any errors. But: it is specifically NOT allowed on windows machines, and there may be other things in between.
There's also some discussion about handling defer Close in this article thats relevant.
From all that, I think its both safe and prudent to (with error checks):
- open the source (read); defer Close
- open the tempfile (write); defer Close
- write to tempfile
- Sync the tempfile
- Close the tempfile
- Close the source file
- rename tempfile over the old file.
The deferred Close's will handle any closing after any other return/error in the process. They'll actually throw an error (see the source) because we're calling Close twice, but we're ok with ignoring it at that point.
There was a problem hiding this comment.
Great article, it helped me understand how we should do it correctly. So according to your suggestions, I added sync and close methods before renaming tempFile. I hope this will be the final solution 😄
There was a problem hiding this comment.
yeah, this has been a great exercise to learn some of these details for me too. Thanks for continuing to tweak!
onyxraven
left a comment
There was a problem hiding this comment.
looks good. I'm gunna add a couple more tests to cover the 'file doesnt exist' cases.
Story/Issue Link
Fixes #22 - sopstool add should add the file to SCM ignores
Background
I like the idea that sopstool is inspired by the blackbox project. One of the blackbox features is handling the .gitignore file.
I think there is no need to deeply explain why it is important. In short, it prevents secrets leaks.
How does it work?
Very simple, it adds the origin filename to .gitingore if you add a new secret and removes the origin filename from .gitignore if you remove a secret. That's all. :)
Versioning
Additional Requests to Reviewers
Tasks