refactor(general): leverage os.CreateTemp
#4513
Conversation
3d37b1b to
41bfdbc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4513 +/- ##
==========================================
+ Coverage 75.86% 76.37% +0.50%
==========================================
Files 470 527 +57
Lines 37301 40044 +2743
==========================================
+ Hits 28299 30584 +2285
- Misses 7071 7444 +373
- Partials 1931 2016 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| fullPath := filepath.Join(tempDirOr(dir), uuid.NewString()) | ||
|
|
||
| f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_EXCL|os.O_RDWR, permissions) //nolint:gosec | ||
| f, err := os.CreateTemp(dir, "kt-") |
There was a problem hiding this comment.
any reason for not using os.CreateTemp here?
There was a problem hiding this comment.
The call to tempDirOr(dir) is redundant, since os.CreateTemp will resolve the actual directory in the same way as tempDirOr was doing it, so the call is no longer necessary.
bc7bb72 to
8604697
Compare
|
@Rohit-BM18 FYI |
| // Create creates a temporary file that will be automatically deleted on close. | ||
| func Create(dir string) (*os.File, error) { | ||
| dir = tempDirOr(dir) | ||
| func CreateAutoDelete() (*os.File, error) { |
There was a problem hiding this comment.
Removed the dir parameter based on @jkowalski suggestion
There was a problem hiding this comment.
And renamed the function to CreateAutoDelete to make it more explicit.
@jkowalski @Rohit-BM18 @redgoat650 if you have a suggestion for a better name, we can change it in a separate PR.
3d61fc6 to
f276f29
Compare
Also, remove the `dir` parameter to `tempfile.CreateAutoDelete`, since only an empty string was passed, apart from test cases.
9260571 to
2965341
Compare
| require.NoError(t, err) | ||
| require.NoError(t, f.Sync()) | ||
|
|
||
| defer f.Close() |
There was a problem hiding this comment.
f is automatically closed and removed on test cleanup
| e := testenv.NewCLITest(t, testenv.RepoFormatNotImportant, runner) | ||
|
|
||
| e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir) | ||
| e.RunAndExpectSuccess(t, "policy", "set", "--global", "--enable-volume-shadow-copy=when-available") |
There was a problem hiding this comment.
relocated here from below, to setup the "repo" first, no functional change to the test.
|
|
||
| oid := sources[0].Snapshots[0].ObjectID | ||
| entries := clitestutil.ListDirectory(t, e, oid) | ||
| t.Log("sources[0].Snapshots[0].ObjectID entries:", entries) |
There was a problem hiding this comment.
This is spurious and can be removed.
| t.Log("sources[0].Snapshots[0].ObjectID entries:", entries) | ||
|
|
||
| if isAdmin { | ||
| require.NotEmpty(t, entries) |
There was a problem hiding this comment.
Before, there was a panic in cases where entries was empty. This explicitly checks the condition and stops the test if the condition is not met making it easier to diagnose the problem.
|
|
||
| if errors.Is(err, syscall.EISDIR) || errors.Is(err, syscall.EOPNOTSUPP) { | ||
| return createUnixFallback(dir) | ||
| return createUnixFallback() |
There was a problem hiding this comment.
This branch is not exercised on CI because the Linux runners have recent enough kernels to succeed in the O_TMPFILE above.
|
@jkowalski @Rohit-BM18 @redgoat650 happy to address feedback in a followup PR. Thanks. |
Use
os.CreateTempwhen creating temporary files intempfile.createUnixFallbackfunction