Use setup-envtest from (test) code about controller-runtime

[alvaroaleman]

Yeah, I agree adding code to the envtest.Environment to download the binaries if needed would be useful

[tomasaschan]

So far I've focused my efforts on lift-and-shifting the reusable packages from tools/setup-envtest to pkg/envtest/setup and re-implementing the workflows with proper error handling (rather than the panic-and-recover based stuff going on in the CLI code...). It's... a bit messy 😅

My plan is to

1. re-implement the workflows as exported functions of the pkg/envtest/setup package
2. make the CLI just use those instead
3. perhaps provide some convenience wrapper on an envtest.Enviroment (or just fallback code) that makes it as easy as possible to run this from tests

and I can easily add

4. replace all usage of Afero with the stdlib fs abstractions

before I submit it for proper review.

[tomasaschan]

Sweet! Then I just have to find time to work on this, too 😅

[sbueringer]

@tomasaschan Just a heads up. We have to migrate the envtest binaries from the GCS bucket to controller-tools releases. This will require some refactoring to setup-envtest to make it possible to pull from this new location. It should be mostly orthogonal to your work, but just wanted to let you know.

I'm working on this now. Probably the location change has to merge first as we probably want to backport it (we don't know at which point the old location will go away, because the GCS bucket is owned by Google and not the community)

[tomasaschan]

@sbueringer Ah, I see. I think that should be fine; I've moved the remote package into controller-runtime proper (so the dependency arrow is setup-envtest -> controller-runtime, not the other way around) but I did it basically without changes, so it should be easy to rebase my changes on top of yours.

The main thing to keep in mind is that there should be a good way to configure a "default" client if the user provides no parameters.

[tomasaschan]

@sbueringer I opened a draft PR with my changes so far, so you can have a look at it if you'd like. Not a whole lot more work to do, but how much time I have to spend on this is a little unpredictable - I might be able to mark it ready for review later today, or not until in a couple of weeks 😅

[fsommar]

Agreed, this would definitely be useful!

[sbueringer]

Also sounds useful to me, we just have to check what the impact on our dependencies is. setup-envtest today is a separate go module.

[tomasaschan]

@sbueringer I started porting code over in my fork, and while it's nowhere near ready to actually review yet (loads of local changes that are in a very unpolished - and uncommitted - state at the moment...) the go.mod changes are probably near done already: there's a few version bumps, and a new dependency on github.com/spf13/afero. Does that sound like a blocker to you?

[sbueringer]

Doesn't look like a showstopper to me. indirect dependency bumps look definitely fine
afero itself only has a few dependencies. Maybe it's worth looking into if we actually need afero. I'm mostly wondering if eventually we could end up in a situation that we don't find compile compatible combinations of k8s.io/* and afero transitive dependencies.

Today we're relatively safe as a vast majority of our dependencies are inherited from k/k. So whenever k/k is able to compile, CR is as well.

[alvaroaleman]

Yeah I'd suggest using the stdlibs fs abstraction rather than afero to avoid the added deps

[tomasaschan]

@sbueringer Will the code remain in the same github repo, or move to its own? I just locally added a dependency from setup-envtest to the main controller-runtime module in order to make the cli use the new implementation; will that, in the long term, be an issue?

I guess the worst case is that my changes will end up needing PRs to multiple repos. We'll cross that bridge when we get there.

[sbueringer]

Will the code remain in the same github repo, or move to its own?

Same repo, same packages. I'll just add an alternate implementation for the client stuff

[sbueringer]

Very early prototype to give you an impression: https://github.com/kubernetes-sigs/controller-runtime/compare/main...sbueringer:controller-runtime:pr-setup-envtest-ct-rel?expand=1

(I'll try to get this done ASAP, hopefully within the next week or so there should be a PR up)

[sbueringer]

Do we still need a separate module for setup-envtest? If it doesn' introduce any additional dependency anymore I would get rid of it entirely (so folks can do go install sigs.k8s.io/controller-runtime/tools/setup-envtest@<CR-version>)

Today people have to pin to commits which is one reason why we were thinking about moving the binary into a separate repository. But If we can get rid of the additional module that all becomes easier.

[tomasaschan]

I don't particularly mind; we might as well use the same module for everything. I (and the kubebuilder init-generated Makefile, which I suppose is not insignificant in this context...) tend to use go install .../setup-envtest@latest anyway... But a big part of my reason to do this work is to not have to install it at all, which makes it matter even less how the modules are structured.

I guess one argument for keeping them separate is that it makes it possible to add some dependencies to setup-envtest that don't have to bloat the dependency tree of controller-runtime itself. For example, the main method is currently quite bloated with both arg parsing and help text etc, and it might be nice to at some point port that to use something like Cobra; but Cobra is currently not a dependency for controller-runtime, so if we keep the modules separate we can do that with smaller regard for CR itself.

[sbueringer]

PR is ready for review: #2811

[sbueringer]

#2811 is merged, so this work should be unblocked now

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[sbueringer]

remove-lifecycle stale