Skip to content
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

refactor: use os.ReadDir for lightweight directory reading #2346

Merged
merged 3 commits into from
Feb 7, 2022
Merged

refactor: use os.ReadDir for lightweight directory reading #2346

merged 3 commits into from
Feb 7, 2022

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Jan 9, 2022

Reference: golang/go#41467

Proposed changes

os.ReadDir was added in Go 1.16 as part of the deprecation of ioutil package. It is a more efficient implementation than ioutil.ReadDir as stated here https://pkg.go.dev/io/ioutil#ReadDir . The full proposal can be read here: https://github.com/golang/go/issues/41467.

This PR also replaces the existing io/ioutil functions with their new definitions in io and os packages, since the io/ioutil package has been deprecated as of Go 1.16, see https://golang.org/doc/go1.16#ioutil.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2022

Codecov Report

Merging #2346 (d9e0bd6) into master (88a255b) will decrease coverage by 0.00%.
The diff coverage is 12.50%.

❗ Current head d9e0bd6 differs from pull request most recent head 5d44c8d. Consider uploading reports for the commit 5d44c8d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2346      +/-   ##
==========================================
- Coverage   53.66%   53.65%   -0.01%     
==========================================
  Files          48       48              
  Lines       14201    14203       +2     
==========================================
  Hits         7621     7621              
- Misses       6340     6342       +2     
  Partials      240      240              
Impacted Files Coverage Δ
internal/metrics/collectors/processes.go 0.00% <0.00%> (ø)
internal/nginx/manager.go 0.00% <0.00%> (ø)
internal/nginx/utils.go 0.00% <0.00%> (ø)
internal/nginx/verify.go 56.92% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88a255b...5d44c8d. Read the comment docs.

@ciarams87 ciarams87 requested review from pleshakov and removed request for soneillf5 January 20, 2022 09:41
Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR and for addressing the linting error. Looks good!

This commit also replaces the existing io/ioutil functions with their
new definitions in io and os packages, since the io/ioutil package has
been deprecated as of Go 1.16, see https://golang.org/doc/go1.16#ioutil.

Reference: https://pkg.go.dev/io/ioutil#ReadDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@@ -56,8 +58,11 @@ func getWorkerProcesses() (int, int, error) {
continue
}

cmdlineFile := fmt.Sprintf("/proc/%v/cmdline", folder.Name())
content, err := ioutil.ReadFile(cmdlineFile)
cmdlineFile := filepath.Clean(fmt.Sprintf("/proc/%v/cmdline", folder.Name()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Juneezee

could you possibly clarify why it is necessary to use filepath.Clean ?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pleshakov Hi, thanks for reviewing this PR.

filepath.Clean is necessary here to address the linting error, see this comment #2346 (comment)

@ciarams87 ciarams87 merged commit 12854a1 into nginxinc:master Feb 7, 2022
@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants