-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
✨ EnvTest Binaries Setup Tool #1488
✨ EnvTest Binaries Setup Tool #1488
Conversation
/assign @joelanford |
e19b89a
to
6ae784e
Compare
Quick FAQ on downloading and "latest":
|
11ac855
to
1326169
Compare
Other notes: this currently writes the archive to a temp file then reads that back, and cleans it up at the end.. Not strictly necessary -- we could stream directly to the gzip -> tar readers, but it theoretically could make debugging easier. 🤷♀️ |
} | ||
|
||
// IsWildcard checks if any part of this version is a wildcard. | ||
func (v Version) IsWildcard() bool { |
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.
Rather than implement a bespoke pseudo-semver system, why not use semver with the following rules:
X.Y.Z
matches an exact version.X.Y
matches latest patch when downloading/switching, or all patches when cleaning.- You could also pin the meaning of
X.Y
to latest patch, and pass--all-patches
toclean
andlist
commands to get wildcard behavior.
- You could also pin the meaning of
I will say though, the wildcard grammar is not complex.
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 wanted a quick way to distinguish "get me the latest" from "get me anything from disk if I have it, otherwise download", but you may be right that it's easier. I'll play around a bit and see if I can find something that works nicely
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.
also, FWIW: all semver selectors are bespoke as far as I can tell -- there doesn't appear to be a spec for the selectors, just the version itself.
refactoring a bit too enable better no-internet support, and to combine commands, etc |
not guarnateed, except that the leaf directory will contain the names | ||
expected by envtest. You should always use `setup-envtest fetch` or | ||
`setup-envtest switch` (generally with the `-p path` or `-p env` flags) to | ||
get the directory that you should use. |
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.
Would not first be required to move forward with #1234?
Is this one working if:
- We remove the bins from: /usr/local/kubebuilder/bin
- Then, use it to install the binaries in the bin directory of the project as we do in KB.
Note that currently on the KB side we can just run the tests with make test and not directly because of it. (https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v3/Makefile#L56)
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 is an alternate approach to installing in GOBIN. We could, for instance, follow up with something along those lines where we have a well-known symlink in the store directory, and have use
or switch
change that instead of having you fiddle with the environment variable, but I figured this was a decent first step that's backwards compatible (all CR versions support using $KUBEBUILDER_ASSETS
)
// | ||
// The results are sorted with newer versions first. | ||
func (c *Client) ListVersions(ctx context.Context) ([]versions.Set, error) { | ||
loc := &url.URL{ |
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.
Can you not use the google storage API 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.
didn't want to pull in a whole bunch of deps for basically one API call, plus testing is a bit easier.
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 can switch it out if you want, but it's not really much code.
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.
Nah that's a fair point.
0cba0b5
to
8d7c266
Compare
Added some tests, made the version selector more semver like:
We can't really use an existing semver library because one of them (blang) is missing support for '~' (maybe not strictly needed?) and the other (Masterminds) has no easy way to construct a version from parts except by round tripping it through a string. We need to keep our own version type because blang's version isnt hashable, and masterminds' is technically hashable but is opaque so it's not guaranteed to stay that way. EDIT: I realize I mis-read what you were suggesting and I guess technically made this more complicated, but at least it's a bit more familiar? And the |
/retest |
I might simplify the selector logic down a bit closer to what you suggested above tomorrow -- right now we handle a lot of cases that don't currently apply (they could in the future, which is why I added them, but 🤷♀️ ) EDIT: simplified the actual code and schema a bit, updated above |
8d7c266
to
614a0ec
Compare
4c19778
to
334c702
Compare
This introduces an envtest binary manager tool. It can download binaries from GCS, list available versions, print out help for switching between them, and remove old ones. By default, it downloads binaries into an OS-specific cache directory, as per the OS's conventions, but this can be overridden.
We can now drop the `setup-envtest.sh` script, and instead just use `source <(setup-envtest fetch -k ${version} -p env)` (plus a couple lines to install setup-envtest ;-)).
334c702
to
782807d
Compare
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, estroz 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 |
Makes use of the new setup-envtest tool (kubernetes-sigs/controller-runtime#1488) in hack/setup-envtest.sh instead of vendoring hack/setup-envtest.sh and fetching binaries with that.
* Upgrade to k/*@v0.21.1 in go.mod * [automated] make revendor for k/* dependencies This deletes pkg/mock/client-go/kubernetes/mocks.go to resolve the following deadlock: make revendor fails because of some dependencies of the file and make generate fails because of missing revendoring. File will be generated again in the next commit. * [automated] make generate for k/* dependencies * Upgrade to c-r@v0.9.0 in go.mod * [automated] make revendor for c-r dependency `make revendor` results in `hack/setup-envtest.sh` being broken, so reset the file after running `make revendor`. Adaption to breaking changes in the upstream file will be done in a later commit. * manager.NewClientBuilder was removed in favor of cluster.DefaultNewClient ref kubernetes-sigs/controller-runtime#1409 * client.*MergeFrom* now take client.Object instead of runtime.Object ref kubernetes-sigs/controller-runtime#1395 * [automated] make generate for c-r dependency * Adapt to changes in labels.NewRequirement ref kubernetes/kubernetes#97538 * Adapt to new setup-envtest tool Makes use of the new setup-envtest tool (kubernetes-sigs/controller-runtime#1488) in hack/setup-envtest.sh instead of vendoring hack/setup-envtest.sh and fetching binaries with that. * [automated] make revendor for setup-envtest tool * Adapt pkg/envtest to upstream changes - Make use of the new Users concept in envtest to provision a dedicated user for gardener-apiserver and a valid kubeconfig - Make use of the new way to configure API server args to easily configure kube-aggregator flags - Also generate certs for aggregation layer on our own instead of reusing the API server ca/certs (which is semantically correct), which allows us to drop our fork including kubernetes-sigs/controller-runtime#1449 * Styling nits
Upstream has replaced the shell script with a single binary. See kubernetes-sigs/controller-runtime#1488 for more information.
* Upgrade to k/*@v0.21.1 in go.mod * [automated] make revendor for k/* dependencies This deletes pkg/mock/client-go/kubernetes/mocks.go to resolve the following deadlock: make revendor fails because of some dependencies of the file and make generate fails because of missing revendoring. File will be generated again in the next commit. * [automated] make generate for k/* dependencies * Upgrade to c-r@v0.9.0 in go.mod * [automated] make revendor for c-r dependency `make revendor` results in `hack/setup-envtest.sh` being broken, so reset the file after running `make revendor`. Adaption to breaking changes in the upstream file will be done in a later commit. * manager.NewClientBuilder was removed in favor of cluster.DefaultNewClient ref kubernetes-sigs/controller-runtime#1409 * client.*MergeFrom* now take client.Object instead of runtime.Object ref kubernetes-sigs/controller-runtime#1395 * [automated] make generate for c-r dependency * Adapt to changes in labels.NewRequirement ref kubernetes/kubernetes#97538 * Adapt to new setup-envtest tool Makes use of the new setup-envtest tool (kubernetes-sigs/controller-runtime#1488) in hack/setup-envtest.sh instead of vendoring hack/setup-envtest.sh and fetching binaries with that. * [automated] make revendor for setup-envtest tool * Adapt pkg/envtest to upstream changes - Make use of the new Users concept in envtest to provision a dedicated user for gardener-apiserver and a valid kubeconfig - Make use of the new way to configure API server args to easily configure kube-aggregator flags - Also generate certs for aggregation layer on our own instead of reusing the API server ca/certs (which is semantically correct), which allows us to drop our fork including kubernetes-sigs/controller-runtime#1449 * Styling nits
* Upgrade to k/*@v0.21.1 in go.mod * [automated] make revendor for k/* dependencies This deletes pkg/mock/client-go/kubernetes/mocks.go to resolve the following deadlock: make revendor fails because of some dependencies of the file and make generate fails because of missing revendoring. File will be generated again in the next commit. * [automated] make generate for k/* dependencies * Upgrade to c-r@v0.9.0 in go.mod * [automated] make revendor for c-r dependency `make revendor` results in `hack/setup-envtest.sh` being broken, so reset the file after running `make revendor`. Adaption to breaking changes in the upstream file will be done in a later commit. * manager.NewClientBuilder was removed in favor of cluster.DefaultNewClient ref kubernetes-sigs/controller-runtime#1409 * client.*MergeFrom* now take client.Object instead of runtime.Object ref kubernetes-sigs/controller-runtime#1395 * [automated] make generate for c-r dependency * Adapt to changes in labels.NewRequirement ref kubernetes/kubernetes#97538 * Adapt to new setup-envtest tool Makes use of the new setup-envtest tool (kubernetes-sigs/controller-runtime#1488) in hack/setup-envtest.sh instead of vendoring hack/setup-envtest.sh and fetching binaries with that. * [automated] make revendor for setup-envtest tool * Adapt pkg/envtest to upstream changes - Make use of the new Users concept in envtest to provision a dedicated user for gardener-apiserver and a valid kubeconfig - Make use of the new way to configure API server args to easily configure kube-aggregator flags - Also generate certs for aggregation layer on our own instead of reusing the API server ca/certs (which is semantically correct), which allows us to drop our fork including kubernetes-sigs/controller-runtime#1449 * Styling nits
This introduces a tool (in Go) to manage installed versions of the envtest binaries.
It can download new ones (both always and if necessary), enumerate installed and available versions, and remove versions.
By default, it installs to an OS-specific cache dir (as per OS conventions), but that can be customized if need-be.
The tool itself is in a separate module, so it can just be
go installed
-ed on 1.16 w/o pulling in extra deps.The README in the subdirectory and the command's
--help
flag have more details, but the TL;DR of usage is:Fixes #1300