-
Notifications
You must be signed in to change notification settings - Fork 54
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
🌱 Configure ENVTEST Binaries for IDE Debugging #1454
base: main
Are you sure you want to change the base?
🌱 Configure ENVTEST Binaries for IDE Debugging #1454
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
6ccb706
to
bec5b49
Compare
# Useful for some CI/CD environments that set neither $XDG_DATA_HOME nor $HOME. | ||
SETUP_ENVTEST_BIN_DIR_OVERRIDE= | ||
ifeq ($(shell [[ $$HOME == "" || $$HOME == "/" ]] && [[ $$XDG_DATA_HOME == "" ]] && echo true ), true) | ||
SETUP_ENVTEST_BIN_DIR_OVERRIDE += --bin-dir /tmp/envtest-binaries |
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 are updating the setup to use the project's bin directory (used already for the other staff), ensuring we don’t interfere with contributors' local development environments."
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.
iirc, the reason this is here is because Red Hat reported issues when they ran CI because of quirks of how their CI system sets up the environment. We would be good stewards if we validated changes in this area in RH's CI system before we make them 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.
Note the conditional: in order for this override to be used we need both:
- $HOME=="" or $HOME=="/"
- XDG_DATA_HOME==""
That will not happen in a developer's local environment.
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.
Thanks for share why it is there
My main idea for local development is to keep the binaries within the project. This way, I can configure ENVTEST properly and debug the tests using the IDE.
I'll take a look into this.
I also don’t want the targets to install anything in my Go workspace or anywhere else outside of the project.
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.
@joelanford Is not only used because the default path will be /usr/local/kubebuilder/, which does not work in the GitHub/ci. If we move to the project's bin dir, would the issues be reported?
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.
Unresolving because I have a related question.
WIth --bin-dir
set to a local directory - do we need a conditional for /tmp/envtest-binaries
?
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.
In my opinion, we don't need /tmp/envtest-binaries
anymore. However, Joe raised concerns about this change, and I agree it's not worth risking breaking downstream CI for now.
Why POV it's no longer needed:
Previously, envtest-setup
installed binaries in $HOME
, which caused issues with GitHub Actions and other CI systems. With the current changes, binaries are now placed in project/bin
, resolving those issues. This makes the temporary /tmp/envtest-binaries
logic unnecessary.
However, that is not the goal of the PR, we can address it in a follow up
That said, I don't want to hold up this PR based on Joe's concerns. It's not the focus of this PR.
We can address this in a follow-up PR to simplify CI by removing the now-unnecessary logic for placing envtest
binaries in /tmp/
.
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.
Is that make sense ?
Can we close this one?
ff5d402
to
94cd7d3
Compare
703e583
to
9ba4f50
Compare
9ba4f50
to
3563506
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1454 +/- ##
==========================================
- Coverage 74.73% 74.69% -0.04%
==========================================
Files 42 42
Lines 3241 3241
==========================================
- Hits 2422 2421 -1
- Misses 646 647 +1
Partials 173 173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
3563506
to
ba71f8b
Compare
Hi @m1kola, Thank you for catching that—really good observation! 🙌 Your comment highlights an additional motivation for this change: addressing scenarios like those outlined in kubernetes-sigs/controller-runtime#2744 and kubernetes-sigs/controller-runtime#2646. The required changes have been addressed, and we no longer use bingo here as intended. Please feel free to review this one. |
ba71f8b
to
70048e4
Compare
I like the integration in TestMain to make envtest work for IDE-based test execution. Could we put that piece in its own PR? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
70048e4
to
038669f
Compare
Hi @joelanford, @m1kola, @perdasilva, @bentito, Apologies for the confusion earlier! This PR is NOT about Bingo. The main goal is to make debugging ENVTEST-based tests in the IDE simpler and more straightforward.
I’ve addressed all comments, closed all as done or outdated to avoid confusion, reverted the BINGO changes, and kept the suggestions. @m1kola, could you please revert your block? Thanks in advance! 😊 |
038669f
to
c5cdc3d
Compare
// | ||
// To ensure the binaries are in the expected path without manual configuration, run: | ||
// `make setup-envtest` | ||
if getFirstFoundEnvTestBinaryDir() != "" { |
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.
Hi @perdasilva,
You added this comment, and I deviated—sorry about that!
Would it be cleaner if we just used an environment variable like
ENV_TEST_BINARY_DIR
or something like that?
Then, it doesn't really matter where the binaries are placed.
We already have an environment variable (KUBEBUILDER_ASSETS
) that can be used, but it’s not very convenient and involves a steep learning curve:
- Know that ENVTEST requires binaries.
- Then, by default, ENVTEST is expected to find the bins in
local/...
. - We can override this using the
KUBEBUILDER_ASSETS
environment variable. - Note that those bins never was in the
$GOPATH/bin
because they are placed via the setup-invest in the global place (see here) which indeed cause issues for those which have restrictive envs. - Then, there’s the added challenge of knowing how to configure the tests properly in an IDE.
This PR simplifies the process so everything “just works” without extra setup or deep knowledge.
Making tests and debugging smoother and hassle-free.
I’ve also updated the code comments to clarify things further.
Hope this clears everything up! 😊
test-unit: $(SETUP_ENVTEST) #HELP Run the unit tests | ||
rm -rf $(COVERAGE_UNIT_DIR) && mkdir -p $(COVERAGE_UNIT_DIR) | ||
eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && \ | ||
KUBEBUILDER_ASSETS="$(shell $(SETUP_ENVTEST) use -p path $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE))" \ |
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.
Hi @m1kola,
Just to clarify, the binaries installed by envtest-setup are no longer placed in the HOME/global directory. See the default behavior here:
https://github.com/kubernetes-sigs/controller-runtime/blob/e3347b5405cdd0da5bff527af3d406117938ba6b/tools/setup-envtest/README.md?plain=1#L57-L70
This change ensures binaries are placed locally, avoiding the conflicts and clutter caused by global installations.
To clarify, I’m referring to the binaries required for ENVTEST (not those managed by BINGO).
Hope this clears things up! 😊
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.
@perdasilva ^ That might clarify better.
test-unit: $(SETUP_ENVTEST) #HELP Run the unit tests | ||
rm -rf $(COVERAGE_UNIT_DIR) && mkdir -p $(COVERAGE_UNIT_DIR) | ||
eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && \ |
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 think this needs to change: to put binaries produced by setup-env into a separate directory - updating SETUP_ENVTEST_BIN_DIR_OVERRIDE
above is enough.
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.
The goal of the change is to simplify.
Make it easier to figure out that the tests use KUBEBUILDER_ASSETS
to set the path for the bins.
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'm on the fence here because of repetition: we would have to now ensure that we pass the same args to both $(SETUP_ENVTEST)
calls. But explicit is better than implicit I guess.
So I'll leave it up to you.
# Useful for some CI/CD environments that set neither $XDG_DATA_HOME nor $HOME. | ||
SETUP_ENVTEST_BIN_DIR_OVERRIDE= | ||
ifeq ($(shell [[ $$HOME == "" || $$HOME == "/" ]] && [[ $$XDG_DATA_HOME == "" ]] && echo true ), true) | ||
SETUP_ENVTEST_BIN_DIR_OVERRIDE += --bin-dir /tmp/envtest-binaries |
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.
Unresolving because I have a related question.
WIth --bin-dir
set to a local directory - do we need a conditional for /tmp/envtest-binaries
?
Hi @m1kola Thank you. Regards #1454 (comment) see : #1454 (comment) That is not the goal of the PR, and this point can be addressed in a follow-up. |
This change introduces downloading the specific binaries required to run ENVTEST-based tests into the project/bin directory. This setup makes it easier to configure tests for debugging directly in an IDE. Furthermore, no longer install binaries required for ENVTEST based tests on the project/bin instead of global path.
c5cdc3d
to
7f4439e
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.
Looks good to me overall.
Just one question about $(SETUP_ENVTEST)
and setup-envtest
targets.
test-unit: $(SETUP_ENVTEST) #HELP Run the unit tests | ||
rm -rf $(COVERAGE_UNIT_DIR) && mkdir -p $(COVERAGE_UNIT_DIR) | ||
eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && \ |
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'm on the fence here because of repetition: we would have to now ensure that we pass the same args to both $(SETUP_ENVTEST)
calls. But explicit is better than implicit I guess.
So I'll leave it up to you.
$(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE) | ||
|
||
.PHONY: test-unit | ||
test-unit: $(SETUP_ENVTEST) setup-envtest #HELP Run the unit tests |
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.
A bit strange to have $(SETUP_ENVTEST)
and setup-envtest
targets.
Is it beneficial to have a separate setup-envtest
target here?
Maybe we can just do this?
test-unit: $(SETUP_ENVTEST) setup-envtest #HELP Run the unit tests | |
test-unit: $(SETUP_ENVTEST) #HELP Run the unit tests | |
mkdir -p $(ROOT_DIR)/bin | |
$(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE) |
This change introduces downloading the specific binaries required to run ENVTEST-based tests into the project/bin directory. This setup makes configuring tests for debugging directly in an IDE easier.
Motivation
BinaryAssetsDirectory
dynamically, contributors can debug tests in their preferred IDEs (e.g., GoLand, VSCode) without requiring additional environment configuration such as settingKUBEBUILDER_ASSETS
.Installing binaries inside the project avoids conflicts with other projects or system-wide installations, particularly useful for contributors with restricted environments (e.g., corporate laptops).
The binaries installed by envtest-setup are no longer placed in the HOME/global directory. See the default behavior here:
https://github.com/kubernetes-sigs/controller-runtime/blob/e3347b5405cdd0da5bff527af3d406117938ba6b/tools/setup-envtest/README.md?plain=1#L57-L70
This change ensures binaries are placed locally, avoiding the conflicts and clutter caused by global installations.
I’m referring to the binaries required for ENVTEST (not those managed by BINGO).
Before this PR those are placed for example in a Mac OS at:
/Users/camilam/Library/Application Support/io.kubebuilder.envtest/k8s/1.31.0-darwin-arm64
KUBEBUILDER_ASSETS
manually.While setting
KUBEBUILDER_ASSETS
is sufficient for experienced contributors, this change makes it easier for newcomers and supports debugging directly in IDEs without additional configuration.