✨ (go/v4): Add new makefile target to create a cluster name to run the e2e tests #4673
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kersten The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @kersten. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
size/L
docs/book/src/cronjob-tutorial/testdata/project/.github/workflows/test-e2e.yml
Outdated
Show resolved
Hide resolved
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
2 times, most recently
from
e87b922 to
c24a8f2
Compare
camilamacedo86
changed the title
| exit 1; \ | ||
| @$(KIND) get clusters | grep -q 'project-test-e2e' || { \ | ||
| echo "Kind cluster not running, creating Kind cluster: project-test-e2e"; \ | ||
| $(KIND) create cluster --name project-test-e2e --wait 5m; \ |
There was a problem hiding this comment.
I the cluster exist already it will fail, so should we not remove it.
I think we will need to clean it up as well, right?
There was a problem hiding this comment.
Great idea! We already have some cleanup logic in our code, I’ll add a dedicated target for it as we have. What do you think - should we clean it up automatically after each test, or just provide a target so it can be run manually? On our side, we usually clean it up manually to avoid long spin-up times during development.
There was a problem hiding this comment.
I think we will need something like
.PHONY: kind-clean
kind-clean:
$(KIND) delete cluster --name $(KIND_CLUSTER_NAME)
and call it at the end of the tests
Also, I think we do not need to check and instead only create the cluster nane
@$(KIND) get clusters | grep -q 'project-test-e2e' || { \
echo "Kind cluster not running, creating Kind cluster: project-test-e2e"; \
$(KIND) create cluster --name project-test-e2e --wait 5m; \
Can we also have an env var like the others for the cluster name?
There was a problem hiding this comment.
I've added that already. And the variable was already requested in this PR: #4617. Could you recheck the changes? There ist a teardown-test-e2e target.
I'll add another change, so that it is execute after the test.
There was a problem hiding this comment.
Hi @kersten
I think the change looks good! We just need to make sure to clean up the cluster after the tests run.
Also, I hope you don’t mind—I updated the title of the PR to make it more suitable for the release notes.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
from
c24a8f2 to
5f5eb82
Compare
| @$(KIND) get clusters | grep -q 'project-test-e2e' || { \ | ||
| echo "Kind cluster not running, creating Kind cluster: project-test-e2e"; \ | ||
| $(KIND) create cluster --name project-test-e2e --wait 5m; \ |
There was a problem hiding this comment.
| @$(KIND) get clusters | grep -q 'project-test-e2e' || { \ | |
| echo "Kind cluster not running, creating Kind cluster: project-test-e2e"; \ | |
| $(KIND) create cluster --name project-test-e2e --wait 5m; \ | |
| $(KIND) create cluster --name $(KIND_CLUSTER_NAME) --wait 5m; \ |
@kersten sorry I might not be very clear.
I think that
a) above we would need to have only the line to create the cluster ( not to check if it exist )
b) use an variable for we store the name of the cluster
Then, after we run the tests
we call a target test-e2e-tweardown or cleanup something like that will delete the cluster name
There was a problem hiding this comment.
For the envvar, I would rather wait for the PR #4617, or would you like me to merge the changes into this one?
There was a problem hiding this comment.
Could we not do all here and keep a full proposal?
WDYT?
There was a problem hiding this comment.
Sure, I will do it later. Probably tonight.
| @$(KIND) get clusters | grep -q 'project-test-e2e' || { \ | ||
| echo "Kind cluster not running, creating Kind cluster: project-test-e2e"; \ |
There was a problem hiding this comment.
| @$(KIND) get clusters | grep -q 'project-test-e2e' || { \ | |
| echo "Kind cluster not running, creating Kind cluster: project-test-e2e"; \ |
Could we remove it?
It has no reason to check if the cluster exists.
If we try to create and itexistst then, the command will fail, so we can try to simplify the syntax and the code
There was a problem hiding this comment.
Ok, removed the check.
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
from
5f5eb82 to
d48303b
Compare
| exit 1; \ | ||
| } | ||
| go test ./test/e2e/ -v -ginkgo.v | ||
| @$(KIND) delete cluster --name project-test-e2e |
There was a problem hiding this comment.
| @$(KIND) delete cluster --name project-test-e2e | |
| @$(KIND) delete cluster --name $(KIND_CLUSTER_NAME) |
Could we use a variable for the cluster name instead?
So, that it is aligned with the other targets , the makefile standard, and to allow users to customize it if they wish to easily?
There was a problem hiding this comment.
Hey, could you take a look at this? #4617
I feel like we might be talking past each other - should I include that change in this PR, or do we want to merge them separately, one after the other?
| @command -v $(KIND) >/dev/null 2>&1 || { \ | ||
| echo "Kind is not installed. Please install Kind manually."; \ | ||
| exit 1; \ | ||
| } | ||
| @$(KIND) get clusters | grep -q 'kind' || { \ | ||
| echo "No Kind cluster is running. Please start a Kind cluster before running the e2e tests."; \ | ||
| $(KIND) create cluster --name project-test-e2e --wait 5m |
There was a problem hiding this comment.
| $(KIND) create cluster --name project-test-e2e --wait 5m | |
| $(KIND) create cluster --name $(KIND_CLUSTER_NAME) |
Could we use a variable for the cluster name instead?
So, that it is aligned with the other targets , the makefile standard, and to allow users to customize it if they wish to easily?
There was a problem hiding this comment.
Also, do we really need wait 5m ?
Why would it be required?
There was a problem hiding this comment.
It doesn't wait at least 5 minutes. As soon as the control plane is ready, it proceeds immediately. We've observed that in rare cases, kind sets up so quickly that deployments to the cluster can fail, causing the test to fail as well. I'd suggest starting with a 5-minute wait. If that turns out to be too short or too long, we can make it configurable later.
There was a problem hiding this comment.
What do you think about starting without a timeout for now, and only adding one later if we run into issues or have a clear reason for it?
For context, I’ve never needed to use a timeout in similar projects so far, so it seems unlikely we’ll need one here either. And in case something does go wrong, it might be better to fail fast and return the error immediately rather than wait up to 5 minutes.
This way, we keep things simple and avoid premature optimization, while still leaving room to adjust if needed later on.
There was a problem hiding this comment.
Hi @kersten,
I'm OK with the proposed changes — I think it's a good addition to the default scaffolds.
I just added a couple of minor suggestions:
a) Use KIND_CLUSTER_NAME to parameterize the cluster name used in the tests
b) Simplify the code by removing the extra check
Thanks for the contribution! 🎉
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
3 times, most recently
from
efd67e7 to
06253d9
Compare
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
4 times, most recently
from
03ba85e to
4acb07d
Compare
| cluster = v | ||
| cluster, ok := os.LookupEnv("KIND_CLUSTER") | ||
| if !ok { | ||
| return fmt.Errorf("KIND_CLUSTER is not set") |
There was a problem hiding this comment.
It would have worked with the old code, but since we no longer have a default kind cluster, it’s not necessary anymore. However, if the environment variable isn’t set, the image deployment will fail. I’d feel more comfortable if we explicitly notify the user when it's missing.
There was a problem hiding this comment.
I will need some time to test this one.
If I just have kind running ( any cluster name ) and run the e2e tests, will that work fine?
If so, I am OK with if not I think we should allow it.
There was a problem hiding this comment.
I get your point. I think you will start the e2e test right out of your IDE. I will have to think about that and implement it. Currently that wouldn't work out of the box. You would have to provide the ENV var in the IDE launch config. I will provide a way, where it defaults to kind.
There was a problem hiding this comment.
Currently that wouldn't work out of the box
Before those changes do you think that it would not also work out of the box?
(PS.: I need to check.)
There was a problem hiding this comment.
Before the change, they've should work out of the box. I reverted.
There was a problem hiding this comment.
So, I think we should revert the change in the utils for now.
If needed, we can revisit and improve or refactor it in a separate PR since it feels a bit out of scope for this one.
That way, we can move forward with the changes in the Makefile without blocking.
WDYT?
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
from
4acb07d to
d131e27
Compare
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
2 times, most recently
from
e853a8d to
4aac705
Compare
| KIND_CLUSTER=$(KIND_CLUSTER) go test ./test/e2e/ -v -ginkgo.v | ||
| $(MAKE) teardown-test-e2e | ||
|
|
||
| .PHONY: teardown-test-e2e |
There was a problem hiding this comment.
After reviewing possible naming options, it seems that cleanup-test-e2e is the most suitable choice.
It aligns well with the existing setup-test-e2e and test-e2e targets, keeping the naming consistent and intuitive. It also clearly communicates its purpose without being overly verbose or ambiguous.
Let me know if you'd prefer a different naming style, but this feels like the most balanced option for now.
There was a problem hiding this comment.
Hi @kersten 👋
Thank you so much for this change, and sorry for the delay in reviewing it!
After going through everything, I believe we're definitely good to move forward — just two small nitpicks to address before merging:
-
Revert the unnecessary change in the
scaffoldsmethod underutils, so that for now we keep only the Makefile improvements.
👉 See: https://github.com/kubernetes-sigs/kubebuilder/pull/4673/files#r2030095359 -
Consider renaming the e2e cleanup test target to
cleanup-test-e2einstead ofteardown-test-e2e.
I think it would simplify the name and align better with the existing naming conventions.
👉 See: https://github.com/kubernetes-sigs/kubebuilder/pull/4673/files#r2030095626
Thanks again for the contribution — really appreciate the effort here! 🙌
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
2 times, most recently
from
1db61f0 to
37a0dd4
Compare
docs/book/src/reference/envtest.md
Outdated
| cluster = v | ||
| cluster, ok := os.LookupEnv("KIND_CLUSTER") | ||
| if !ok { | ||
| return fmt.Errorf("KIND_CLUSTER is not set") |
There was a problem hiding this comment.
Hey! Just wondering — could we not keep it as before, so that if KIND_CLUSTER isn’t set, it still works with the default kind context? We could revisit this in a follow-up if needed.
WDYT? Just trying to keep it working out of the box for folks not setting that env var. 🙏
change default Kind cluster names to be project-specific for e2e tests Previously, the default Kind cluster name was "kind", which could lead to conflicts when running multiple e2e tests concurrently across different projects. This change updates the Kind cluster name to be project-specific, reducing potential conflicts and improving the isolation of e2e test environments.
kersten
force-pushed
the
feat/project-specific-kind-cluster
branch
from
37a0dd4 to
2bbb292
Compare
change default Kind cluster names to be project-specific for e2e tests
Previously, the default Kind cluster name was "kind", which could lead to conflicts when running multiple e2e tests concurrently across different projects. This change updates the Kind cluster name to be project-specific, reducing potential conflicts and improving the isolation of e2e test environments.