Skip to content

Conversation

@thaJeztah
Copy link
Member

I had a "WIP" branch for this locally, and decided to finish the work; there's quite some uses of this package, so I split this up into commits per-package for easier reviewing.

Current versions of Go also added t.TempDIr() and t.Cleanup() for tests, so while at it, I updated tests to use those (which simplified the tests).

Finally, I updated the golangci-lint configuration to prevent the package from accidentally being added again.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
The package has been deprecated since Go 1.16:

https://go.dev/doc/go1.16#ioutil

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Feb 25, 2022
@thaJeztah thaJeztah added this to the 21.xx milestone Feb 25, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2022

Codecov Report

Merging #3443 (8c3ae38) into master (df7adf4) will not change coverage.
The diff coverage is 70.21%.

@@           Coverage Diff           @@
##           master    #3443   +/-   ##
=======================================
  Coverage   58.24%   58.24%           
=======================================
  Files         287      287           
  Lines       24155    24155           
=======================================
  Hits        14070    14070           
  Misses       9225     9225           
  Partials      860      860           

@thaJeztah
Copy link
Member Author

When reviewing, I recommend doing so per commit (as github doesn't like showing the whole diff 😅) most of them should be easy to review, but perhaps some may need closer looking (where I refactored or removed some test-utils for t.TempDir() etc.

@thaJeztah
Copy link
Member Author

@silvin-lubecki @ndeloof @crazy-max ptal 🤗

Comment on lines +37 to +43
depguard:
list-type: blacklist
include-go-root: true
packages:
# The io/ioutil package has been deprecated.
# https://go.dev/doc/go1.16#ioutil
- io/ioutil
Copy link
Member

Choose a reason for hiding this comment

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

we should have that in buildkit too (cc @tonistiigi)

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM
I like use of t.TempDir() that don't force us to manage a cleanup function 👍

@thaJeztah
Copy link
Member Author

Yes! t.TempDir() is really nice!!

I also want to have a look at uses of defer altogether in tests (not really a priority though), and perhaps change them to use t.Cleanup() instead. Probably doesn't make a lot of difference, but t.Cleanup() was designed for that, so perhaps there's advantages (e.g. perhaps it takes time taken by those into account when reporting test-durations etc)

@thaJeztah
Copy link
Member Author

ok, let me get this one in; thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants