fix(provisioner/terraform/cleanup): use mtime instead of atime #10892
Conversation
johnstcn
commented
Nov 27, 2023
•
- Updates existing unit test to use a real filesystem as Afero's in-memory FS doesn't support atimes at all
- Updates plugin staleness check to check mtime instead of atime, as atime has been shown to be unreliable
| require.NoError(t, err, "can't stat parent") | ||
| if parentInfo.ModTime().After(mtime) { | ||
| require.NoError(t, fs.Chtimes(parent, atime, mtime), "can't set mtime of parent to match child") | ||
| } |
There was a problem hiding this comment.
review: bit hacky but we want this to look as close to reality as possible
There was a problem hiding this comment.
just double-check: this should work consistency for different OSes? no risk of flakiness?
There was a problem hiding this comment.
We use a different tempdir for each test and perform the file operations in serial, so it should probably be OK.
| addPluginFolder(t, fs, coderPluginPath, "new_folder", now, now.Add(-43*24*time.Hour)) | ||
| addPluginFile(t, fs, coderPluginPath, filepath.Join("new_folder", "foobar.tf"), now, now.Add(-4*time.Hour)) // touched |
There was a problem hiding this comment.
I think the intent of this test was to ensure that a plugin didn't get marked as stale if a file within its folder got touched, but the test was touching a directory. Updated to match.
| fs := afero.NewMemMapFs() | ||
| now := time.Date(2023, time.June, 3, 4, 5, 6, 0, time.UTC) | ||
| prepare := func() (afero.Fs, slog.Logger) { | ||
| // afero.MemMapFs does not modify atimes, so use a real FS instead. |
| require.NoError(t, err, "can't stat parent") | ||
| if parentInfo.ModTime().After(mtime) { | ||
| require.NoError(t, fs.Chtimes(parent, atime, mtime), "can't set mtime of parent to match child") | ||
| } |
There was a problem hiding this comment.
just double-check: this should work consistency for different OSes? no risk of flakiness?
| @@ -7,7 +7,6 @@ import ( | |||
| "strings" | |||
| "time" | |||
|
|
|||
| "github.com/djherbis/times" | |||
There was a problem hiding this comment.
Ah, still referenced in provisionersdk Sessions cleanup. I'll do a separate PR for this.