-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Replace ioutil with os/io packages, run gofmt #5533
🌱 Replace ioutil with os/io packages, run gofmt #5533
Conversation
Welcome @ahrtr! |
Hi @ahrtr. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
acbe0fb
to
bf4cf04
Compare
/ok-to-test |
/retitle 🌱 Replace ioutil with os and io |
… we need to replace ioutil with os. A couple of gofmt warnings are also fixed in this PR.
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.
Will a go mod why
work for a built in package? Just trying to make sure we have all occurrences within our own packages
Alternatively, is there a one shot we can run to verify that none of the files are importing this package now?
@@ -16,5 +16,5 @@ limitations under the License. | |||
|
|||
package v1beta1 | |||
|
|||
func (*DockerMachinePool) Hub() {} | |||
func (*DockerMachinePoolList) Hub() {} | |||
func (*DockerMachinePool) Hub() {} |
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.
This change seems unrelated to this PR?
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.
YES, it's fixed automatically by gofmt -w .
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 wonder why we're not enforcing gofmt on this file
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.
might be the version rendering differently
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 configure my local go version to 1.16.7 on purpose, so as to be consistent with the version in go.mod.
$ go version
go version go1.16.7 darwin/amd64
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 think the go version is fine (otherwise the verify job would fail).
I think gofmt is not enforced in the test go module (same for hacks)
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 just ran gofmt.-w .
on a linux vm, and it has the same result as my Mac.
$ go version
go version go1.16.9 linux/amd64
@@ -22,10 +22,10 @@ package tools | |||
import ( | |||
_ "github.com/drone/envsubst/v2/cmd/envsubst" | |||
_ "github.com/joelanford/go-apidiff" | |||
_ "github.com/mikefarah/yq/v4" |
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.
This change seems unrelated to this PR?
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.
YES, it's fixed automatically by gofmt -w .
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.
gofmt
usually doesn't reorder imports, are you using goimports
instead or some other formatter?
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 does:
$ which gofmt
/usr/local/opt/go@1.16/bin/gofmt
gofmt -w ./hack/tools/tools.go
git diff
diff --git a/hack/tools/tools.go b/hack/tools/tools.go
index 792e899d1..248cc707c 100644
--- a/hack/tools/tools.go
+++ b/hack/tools/tools.go
@@ -22,10 +22,10 @@ package tools
import (
_ "github.com/drone/envsubst/v2/cmd/envsubst"
_ "github.com/joelanford/go-apidiff"
+ _ "github.com/mikefarah/yq/v4"
_ "github.com/onsi/ginkgo/ginkgo"
_ "gotest.tools/gotestsum"
_ "k8s.io/code-generator/cmd/conversion-gen"
_ "sigs.k8s.io/controller-runtime/tools/setup-envtest"
_ "sigs.k8s.io/controller-tools/cmd/controller-gen"
- _ "github.com/mikefarah/yq/v4"
)
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 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 also thought that only goimports does anything to the imports.
bf4cf04
to
d0cefca
Compare
/lgtm |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retitle 🌱 Replace ioutil with os/io packages, run gofmt |
The ioutil package has already been deprecated in golang 1.16, please see go1.16#ioutil. So we need to replace all the ioutil with os and io.
This PR also contains two changes made by gofmt automatically.