-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Updating minimum Go version to 1.19 #3304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d6277f9
to
6078378
Compare
bc2f0ba
to
f4c2421
Compare
container/docker/handler_test.go
Outdated
testDir, err := ioutil.TempDir("", "") | ||
testDir, err := os.MkdirTemp(os.TempDir(), "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to use t.TemDir()
here.
fs/fs_test.go
Outdated
@@ -119,14 +118,14 @@ func TestDirDiskUsage(t *testing.T) { | |||
as := assert.New(t) | |||
fsInfo, err := NewFsInfo(Context{}) | |||
as.NoError(err) | |||
dir, err := ioutil.TempDir(os.TempDir(), "") | |||
dir, err := os.MkdirTemp(os.TempDir(), "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use t.TempDir()
here.
@@ -279,7 +290,23 @@ func (fs *realSysFs) GetNetworkStatValue(dev string, stat string) (uint64, error | |||
|
|||
func (fs *realSysFs) GetCaches(id int) ([]os.FileInfo, error) { | |||
cpuPath := fmt.Sprintf("%s%d/cache", cacheDir, id) | |||
return ioutil.ReadDir(cpuPath) | |||
dir, err := os.ReadDir(cpuPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is an incompatible change, but it would make sense to convert this to return []os.DirEntry
instead.
Otherwise, practically, what happens here, is a stat(2)
call to every entry, which is kinda expensive. More to say, it seems the only user of this function is in utils/sysinfo (I checked), so while this is a public API, apparently it has no users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three other uses of toFileInfo()
that would have to be fixed. While I understand you concern and I believe that it's valid, I'm hesitant to do this as part of this PR and I would prefer to address it in future.
/assign @bobbypage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Will you handle the @kolyshkin proposal on the t.TempDir
?
I did in 814bb27 |
I miss that, sorry |
delayed lgtm, thank you for the fixes! |
Reasoning behind this proposal:
1.20
onmain
anyway1.19
and1.20
only/
and/cmd
shall be the same.Note for the reviewers: take a look at
utils.sysfs.toFileInfo()
- this the only addition/change that was not performed using replace functionality in my IDE.