🐛 EnsureEmptyDirectory should recursively set writable perms prior to delete #1691
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
joelanford
changed the title
joelanford
force-pushed
the
fix-cache-delete
branch
from
3154920 to
70e97fd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1691 +/- ##
==========================================
- Coverage 67.74% 67.72% -0.03%
==========================================
Files 57 57
Lines 4620 4620
==========================================
- Hits 3130 3129 -1
- Misses 1265 1266 +1
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| // DeleteReadOnlyRecursive deletes read-only directory with path given by `root` | ||
| func DeleteReadOnlyRecursive(root string) error { |
There was a problem hiding this comment.
nit: would be nice to have the prerequisite of setting to writable mentioned in the doc
There was a problem hiding this comment.
Setting to writable is not a prerequisite of this function. This function will attempt to set everything writable before attempting to delete.
Regardless, this PR is mainly about moving some existing code around to resolve a bug with EnsureEmptyDirectory, so I'd like to defer nice-to-have changes to a follow-up.
There was a problem hiding this comment.
- setting everything writable is a prerequisite for actually recursively deleting root, which is what the function name and its doc comment says it will do
- it would be nice to include that in the doc comment, that shouldn't be longer than 1 line
Still it's a nit, so feel free to go with whatever you think works best
internal/fsutil/helpers_test.go
Outdated
| require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerReadOnlyFileMode)) | ||
| require.NoError(t, os.Chmod(nestedDir, ownerReadOnlyDirMode)) | ||
|
|
||
| t.Log("Set directory structure as read-only") |
There was a problem hiding this comment.
nit: call DeleteReadOnlyRecursive
There was a problem hiding this comment.
I call it in line 136. Is that what you were asking for?
There was a problem hiding this comment.
I meant that log comment should reflect the call
There was a problem hiding this comment.
Ah, I see. I added via DeleteReadOnlyRecursive in the latest push.
joelanford
force-pushed
the
fix-cache-delete
branch
from
70e97fd to
c602f87
Compare
…elete Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
fix-cache-delete
branch
from
c602f87 to
02b14c6
Compare
|
/lgtm |
|
@joelanford mentioned this fix is required because If there are read-only files or directories in the cache dir when the process starts up (which can easily happen in catalogd), then we can't delete them and error out instead. This fix tries to set the files writable before deleting them. |
|
Thanks for the update to the description @LalatenduMohanty |
f77ebfa
This fix is required because If there are read-only files or directories in the cache dir when the process starts up (which can easily happen in catalogd), then we can't delete them and error out instead. This fix tries to set the files writable before deleting them.
Reviewer Checklist