-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
🐛 (v3-alpha) Makefile: fix source error, improve output text #1725
🐛 (v3-alpha) Makefile: fix source error, improve output text #1725
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
4b3a78b
to
f073de7
Compare
45635df
to
2847e10
Compare
…ror, improve output text
2847e10
to
45eb563
Compare
fetch_envtest_tools $(ENVTEST_ASSETS_DIR) && \ | ||
setup_envtest_env $(PWD)/$(ENVTEST_ASSETS_DIR) && \ | ||
go test ./... -coverprofile cover.out ; \ | ||
} |
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 @estroz,
Since it is more a @joelanford's idea. I think would be nice ping him here to see what he thinks about that.
/cc @joelanford |
This would be better fixed by kubernetes-sigs/controller-runtime#1258. If/when that PR gets merged I will updated this one with the improved script usage. /hold |
@estroz: PR needs rebase. 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. |
fetch_envtest_tools $(ENVTEST_ASSETS_DIR) && \ | ||
setup_envtest_env $(PWD)/$(ENVTEST_ASSETS_DIR) && \ | ||
go test ./... -coverprofile cover.out ; \ | ||
} |
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.
My main concerns with this current solution are:
- Users are unable to run the tests to troubleshooting via delve or an IDE since the test will ONLY work if we execute them via the source.
- The target is complex and not too much unstable which reduces it maintainability
- Its dependency with controller-runtime which make harder we keep it maintained. See that controller-runtime is only a framework and who install/use the bins is ONLY the kubebuilder. So, IMO its implementation should be done in Kubebuilder side. Note that if we want to change something on the steps required to get the binaries we need to bump a new version of the controller-runtime and wait for the next release whcih for example do not allow we do it for v2+ since controller-runtime has breaking changes in the upper version.
IMO: I prefer the approach done in the PR #1711 which solves all problems described above.
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.
responded on #1711
@estroz: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
The
test
rule uses thesource
command which is abash
util, but the default shell is/bin/sh
which may not have this function. Explicitly invoking commands withbash
in these recipes guarantees that this command, and others, are available.Additionally this target prints a bunch of commands unnecessarily. Using a make condition stops this from happening.
/kind bug