-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Vendor gazelle and kazel #57600
Vendor gazelle and kazel #57600
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,13 +26,14 @@ kube::util::ensure-gnu-sed | |
# TODO(spxtr): Remove this line once Bazel is the only way to build. | ||
rm -f "${KUBE_ROOT}/pkg/generated/openapi/zz_generated.openapi.go" | ||
|
||
# The git commit sha1s here should match the values in $KUBE_ROOT/WORKSPACE. | ||
kube::util::go_install_from_commit \ | ||
github.com/kubernetes/repo-infra/kazel \ | ||
97099dccc8807e9159dc28f374a8f0602cab07e1 | ||
kube::util::go_install_from_commit \ | ||
github.com/bazelbuild/bazel-gazelle/cmd/gazelle \ | ||
0.10.1 | ||
# Ensure that we find the binaries we build before anything else. | ||
export GOBIN="${KUBE_OUTPUT_BINPATH}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to export this? I don't like modifying a user's environment beyond the life of the script. There's a couple other cases that you're doing this too, so I'm not sure if it's strictly a requirement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
PATH="${GOBIN}:${PATH}" | ||
|
||
# Install tools we need, but only from vendor/... | ||
go install ./vendor/github.com/bazelbuild/bazel-gazelle/cmd/gazelle | ||
|
||
go install ./vendor/github.com/kubernetes/repo-infra/kazel | ||
|
||
touch "${KUBE_ROOT}/vendor/BUILD" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,7 @@ _kubetmp="${_kubetmp}/kubernetes" | |
|
||
# Do all our work in the new GOPATH | ||
export GOPATH="${_tmpdir}" | ||
export PATH="${GOPATH}/bin:${PATH}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You had raised concerns about doing stuff like this here: #59048 (comment) I'm wondering if a method like #59210 would be better. This approach uses the run-in-gopath method so that the utility is built hermetically, and doesn't A) overwrite a version of the tool already existing in the user's environment or B) pollute their $GOPATH/pkg Let me know what you think! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue I took was changing global variables deep inside a function. Changing them in a first-level script is OK. Basically, any place that changes GOPATH should also change PATH to frontload the new GOPATH/bin There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a commit here that seems simple - PTAL? |
||
|
||
pushd "${_kubetmp}" > /dev/null 2>&1 | ||
# Restore the Godeps into our temp directory | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ openapi_library( | |
"pkg/kubelet/apis/kubeletconfig/v1beta1", | ||
"pkg/proxy/apis/kubeproxyconfig/v1alpha1", | ||
"pkg/version", | ||
"vendor/github.com/kubernetes/repo-infra/kazel", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. welp, the openapi generator in kazel is picking up itself, erroneously. I'll fix this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks |
||
], | ||
tags = ["automanaged"], | ||
vendor_prefix = openapi_vendor_prefix, | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 don't like that there is inconsistency between the way we handle godep, and the way we are handling gazelle/kazel/misspell.
It's not enough of a dislike to block this PR that we badly need, but I'd really like if we could unify on a single way (maybe via a helper util function) to install a specific version of a go app from vendor, use it in the context of the script, with as little pollution of the environment as possible. Maybe I'll look at this in a follow up 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.
We could set GOBIN in init.sh or something, and then always call $GOBIN/godep
But as long as we're checking the version of godep, it shouldn't matter. I agree it would be cleaner to be consistent. Our scripts are a mess here.
alternatively, make teh callers of
ensure_godep_version
pass the path to the godep they are using, and they can pass${GOBIN}/godep
?Agree it should be a followup