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.

Sign up for GitHub

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

Jump to bottom

refactor: move from io/ioutil to io and os package #9811

Merged
merged 1 commit into from
Sep 28, 2021
Merged

refactor: move from io/ioutil to io and os package #9811

merged 1 commit into from
Sep 28, 2021

Conversation

Juneezee
Copy link
Contributor

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

Partly related to #9642

Hi! The io/ioutil package has been deprecated in Go 1.16 (See https://golang.org/doc/go1.16#ioutil). This PR replaces the existing io/ioutil functions with their new definitions in io and os packages.

Special notes for reviewers

No breaking changes.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 23, 2021
@powersj powersj self-requested a review September 23, 2021 17:56
@powersj
Copy link
Contributor

powersj commented Sep 23, 2021

@Juneezee Thanks for this! It does look like there were some failed lint issues:

Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `io.ReadAll` is not checked (errcheck) 
Error: Error return value of `io.ReadAll` is not checked (errcheck)

Would you mind getting the tests green and I can go through and verify the correct updates were made in a review?

Thanks again!

Juneezee
Juneezee commented Sep 23, 2021
View reviewed changes
internal/process/process.go Outdated
Comment on lines 190 to 189
io.Copy(ioutil.Discard, r)
io.Copy(io.Discard, r)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@powersj Hi, thanks for taking the time to review this PR. I believe these lint errors have existed before this PR, and they are reported because lines like these have been modified in this PR.

To make the lint passes, the easiest way is to ignore the error. I'll force push a new commit now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a side note, the screenshot below shows some of the lint errors reported by golangci-lint. These errors will be ted by the lint-pr-changes job when the corresponding lines are modified.
2021-09-24_02-38

internal/process/process.go
Comment on lines -190 to +189
io.Copy(ioutil.Discard, r)
_, _ = io.Copy(io.Discard, r)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for lint error 1

plugins/outputs/opentsdb/opentsdb_http.go
Comment on lines -166 to +165
io.Copy(ioutil.Discard, resp.Body)
_, _ = io.Copy(io.Discard, resp.Body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for lint error 2

plugins/common/shim/input_test.go
Comment on lines -48 to +49
go ioutil.ReadAll(r)
go func() {
_, _ = io.ReadAll(r)
}()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for lint error 3

plugins/common/shim/processor_test.go
Comment on lines -87 to +88
go ioutil.ReadAll(r)
go func() {
_, _ = io.ReadAll(r)
}()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for lint error 4

@Juneezee
Copy link
Contributor Author

@powersj Please take a look again. Many thanks 😄

powersj
powersj approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

+1 on the changes. Verified the branch no longer has any references to ioutil, ensured that the changes were made according to the deprecation notice, and that the binary built and a basic config ran.

Marking ready for final review.

@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 23, 2021
@Juneezee
refactor: move from io/ioutil to io and os package
2af3105 
The io/ioutil package has been deprecated as of Go 1.16, see
https://golang.org/doc/go1.16#ioutil. This commit replaces the existing
io/ioutil functions with their new definitions in io and os packages.

Signed-off-by: Eng Zer Jun <[email protected]>
@telegraf-tiger
Copy link
Contributor

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

@MyaLongmire MyaLongmire merged commit 6a3b271 into influxdata:master Sep 28, 2021
reimda pushed a commit that referenced this pull request Oct 6, 2021
@Juneezee @reimda
refactor: move from io/ioutil to io and os package (#9811)
4956994 
(cherry picked from commit 6a3b271)
@popey popey mentioned this pull request Oct 14, 2021
Closed
aihessu added a commit to aiven/telegraf that referenced this pull request Dec 9, 2021
@aihessu
Revert "refactor: move from io/ioutil to io and os package (influxdat…
61f9287 
…a#9811)"

This reverts commit 4956994.

Enable building on golang 1.15 again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@powersj powersj powersj approved these changes

Assignees
No one assigned
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Juneezee @powersj @MyaLongmire