Use the tool directive instead of tools.go without sharing dependency in hack/tools #133315
Conversation
|
Hi @kei01234kei. Thanks for your PR. I'm waiting for a kubernetes 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-sigs/prow repository. |
|
/assign |
| sigs.k8s.io/yaml v1.6.0 // indirect | ||
| ) | ||
|
|
||
| tool ( |
There was a problem hiding this comment.
I did like the comments on the tools.go imports explaining the use... any chance we can include comments here as well?
There was a problem hiding this comment.
Unfortunately, we can't add comments like this because go mod tidy automatically sorts the dependencies alphabetically, which messes up the comment grouping.
tool (
// linting tools
github.com/aojea/sloppy-netparser
github.com/golangci/misspell
github.com/jcchavezs/porto/cmd/porto
honnef.co/go/tools/cmd/staticcheck
// benchmarking tools
github.com/cespare/prettybench
gotest.tools/gotestsum
...
)While we can add comments in this format, it feels like go.mod isn't the right place for human-written comments anyway. If we want to keep notes on these tools, it would probably be better to put them in a README.md file under hack/tools.
tool (
github.com/aojea/sloppy-netparser // linting tools
github.com/cespare/prettybench // benchmarking tools
github.com/golangci/misspell // linting tools
)There was a problem hiding this comment.
that's too bad. but at least we can just search for scripts referencing these paths
|
I'm not sure what "Don't share the tool dependencies" means ... the dependencies are still shared, I think |
This means we should avoid using the tool directive in the repository's root go.mod file so that tool dependencies are not shared with the main Kubernetes module's dependencies. |
|
|
||
| tool ( | ||
| github.com/aojea/sloppy-netparser | ||
| github.com/cespare/prettybench |
There was a problem hiding this comment.
wait... does this lose us the ability to specify which version of a tool we want to use? I wouldn't have expected any versions in the go.mod file or go.sum file to change if all we're doing is reorganizing where we record the tools / versions
There was a problem hiding this comment.
I wouldn't have expected any versions in the go.mod file or go.sum file to change if all we're doing is reorganizing where we record the tools / versions
You're right. This PR initially also updated the tool modules. That happened because I didn't specify their versions when moving them to the tool directive, so Go defaulted to fetching the latest ones.
I've already fixed this in the latest commit by reverting them to their original versions.
install appropriate version
34f8233 to
e39983c
Compare
e39983c to
a2061e9
Compare
|
/ok-to-test |
|
/sig architecture this lgtm, will see how happy CI is |
|
/retest |
|
/lgtm |
|
LGTM label has been added. Git tree hash: f3986e88ac705a913b0ffdfa389f87b56c90e4ba
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kei01234kei, liggitt 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 |
|
/kind cleanup |
|
/sig testing |
|
/hold |
|
/unhold |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR is related to:
#133316
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: