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: move from io/ioutil to io and os package #3294

Merged
merged 2 commits into from
Oct 2, 2021
Merged

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

merged 2 commits into from
Oct 2, 2021

Conversation

Juneezee
Copy link
Contributor

Which problem is this PR solving?

Remove all usage of io/ioutil functions

Short description of the changes

The io/ioutil package has been deprecated in Go 1.16 (See https://golang.org/doc/go1.16#ioutil). Since Jaeger has upgraded to Go 1.17 (#3220), this PR replaces the existing io/ioutil functions with their new definitions in io and os packages.

@Juneezee Juneezee requested a review from a team as a code owner September 29, 2021 14:34
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #3294 (1cd47d7) into master (c1db700) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3294      +/-   ##
==========================================
+ Coverage   95.86%   95.98%   +0.11%     
==========================================
  Files         259      259              
  Lines       15413    15413              
==========================================
+ Hits        14776    14794      +18     
+ Misses        548      525      -23     
- Partials       89       94       +5     
Impacted Files Coverage Δ
cmd/anonymizer/app/anonymizer/anonymizer.go 83.76% <100.00%> (+19.65%) ⬆️
cmd/collector/app/handler/http_handler.go 100.00% <100.00%> (ø)
cmd/collector/app/zipkin/http_handler.go 97.61% <100.00%> (ø)
cmd/query/app/static_handler.go 95.80% <100.00%> (-1.80%) ⬇️
cmd/status/command.go 100.00% <100.00%> (ø)
crossdock/services/agent.go 100.00% <100.00%> (ø)
crossdock/services/query.go 100.00% <100.00%> (ø)
pkg/config/tlscfg/options.go 100.00% <100.00%> (ø)
pkg/es/client/client.go 87.03% <100.00%> (ø)
...in/sampling/strategystore/static/strategy_store.go 97.26% <100.00%> (ø)
... and 5 more

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 0bdcec8...1cd47d7. Read the comment docs.

albertteoh
albertteoh previously approved these changes Oct 1, 2021
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

lgtm

@yurishkuro
Copy link
Member

@albertteoh can you check why code cov is down? I'd expect refactoring not to do that.

@albertteoh
Copy link
Contributor

@albertteoh can you check why code cov is down? I'd expect refactoring not to do that.

It's because, unfortunately, those lines (cmd/anonymizer/app/anonymizer/anonymizer.go lines 83 & 110) were not covered in the first place (the red gutters indicate no coverage):

Screen Shot 2021-10-02 at 4 47 41 pm

Screen Shot 2021-10-02 at 4 48 01 pm

Code cov's coverage metric is computed off the diff, which means the refactorings touching these few lines uncovered can cause a PR to not pass the 95% diff coverage threshold.

We'd need to add some basic tests for these lines. @Juneezee, would you mind adding some tests to anonymizer's New and SaveMapping functions?

@Juneezee
Copy link
Contributor Author

Juneezee commented Oct 2, 2021

@albertteoh No problem. I will work on it.

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]>
@albertteoh
Copy link
Contributor

Thanks @Juneezee, please rebase as well.

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

}

for _, tt := range tests {
tt := tt
Copy link
Contributor

@albertteoh albertteoh Oct 2, 2021

Choose a reason for hiding this comment

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

nit: this will work but I don't think it's necessary; it's really only useful when referring to its pointer reference or if being used concurrently.

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Thanks!

@albertteoh albertteoh merged commit a883aea into jaegertracing:master Oct 2, 2021
@joe-elliott joe-elliott added this to the v1.27.0 milestone Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants