-
Notifications
You must be signed in to change notification settings - Fork 21
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
Improve tooling installation #24
Conversation
mrueg
commented
Jan 26, 2023
- Update tools to latest version
- Don't repeat ourselves in the install.sh
Hi @mrueg. 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. |
/assign @ehashman |
tools/install.sh
Outdated
sigs.k8s.io/controller-tools/cmd/controller-gen \ | ||
github.com/golangci/golangci-lint/cmd/golangci-lint \ | ||
sigs.k8s.io/kustomize/kustomize/v3 | ||
cat tools.go | grep _ | awk -F'"' '{print $2}' | xargs -tI % go install % |
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 understand what this does but this sacrifices clarity for reusability, can you add a brief comment explaining what this does? The grep expression is also very broad, might want to scope it down. e.g.
cat tools.go | grep _ | awk -F'"' '{print $2}' | xargs -tI % go install % | |
# Find import lines matching _, drop the _ and whitespace, pass as arguments to go install | |
grep '^\s*_' tools.go | awk '{print $2}' | xargs -t go install |
Changes I made in the suggestion here:
- cat isn't necessary
- reduced scope of grep to only match lines that start with possible whitespace then _
- no need to set the field separator to ", the shell will handle them fine
- pass all lines directly to a single
go install
command rather than installing each dep separately, which preserves the current behaviour of this script
This way you only need to add them to tools.go Co-Authored-By: Elana Hashman <ehashman@apple.com>
pkg/collector/api/api.pb.go
Outdated
// protoc-gen-go v1.28.0 | ||
// protoc v3.21.12 | ||
// protoc-gen-go v1.28.1 | ||
// protoc v3.21.9 |
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.
Not sure why this downgraded here.
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.
@mrueg I set up the CI installer to use 21.21, if this is regenerating locally with a lower version then your local install might be behind? As is this will fail the vet CI job.
PB_VERSION=${PB_VERSION:-21.12} |
@@ -121,46 +111,45 @@ require ( | |||
github.com/lufeee/execinquery v1.2.1 // indirect | |||
github.com/magiconair/properties v1.8.6 // indirect | |||
github.com/mailru/easyjson v0.7.6 // indirect | |||
github.com/maratori/testableexamples v1.0.0 // indirect |
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.
MIT - what is pulling this in?
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.
./github.com/golangci/golangci-lint/pkg/golinters/testpackage.go: "github.com/maratori/testpackage/pkg/testpackage"
./github.com/golangci/golangci-lint/pkg/golinters/testableexamples.go: "github.com/maratori/testableexamples/pkg/testableexamples"
pkg/collector/api/api.pb.go
Outdated
// protoc-gen-go v1.28.0 | ||
// protoc v3.21.12 | ||
// protoc-gen-go v1.28.1 | ||
// protoc v3.21.9 |
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.
@mrueg I set up the CI installer to use 21.21, if this is regenerating locally with a lower version then your local install might be behind? As is this will fail the vet CI job.
PB_VERSION=${PB_VERSION:-21.12} |
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.
/lgtm
@ndipebot: changing LGTM is restricted to collaborators In response to this:
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. |
neat trick |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrueg, ndipebot, pwittrock 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 |