-
Notifications
You must be signed in to change notification settings - Fork 86
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
chore: use setup-envtest to bootstrap EnvTest #431
Conversation
c27a621
to
fd2b3be
Compare
I'm confused why this is passing tests... but it could be platform differences. I am unable to run
I will look into ARM options for around this timeframe. This likely predates Mac M1. |
this is part of the issue with having the build tool manage these details. It is great for evergreen, leading edge dev. That said, I'm a fail of env-test (which was influx around M1 and over multiple versions of k8s). I think most of that has been worked out. I should add, I had issues local with integration tests as well... but for the same reason. |
btw
😞 |
Maybe upgrade K8s to 1.24.2 before merging this then? |
@erikgb for awareness... I'm working to upgrade our CRDs to V1 then moving to the latest version of testenv. Once this is in place, I believe we will be ready to merge this. |
ec6b2a6
to
b398a5d
Compare
@kensipe Thanks for fixing the requirements to go beyond K8s version 1.19! Somehow I cannot run the integration tests locally now, even with K8s version 1.19.2. It just hangs, and that worked before rebasing the PR. 🤔 But CI is happy, so I suggest moving forward. Curious if running the integration tests works for you now? |
b398a5d
to
16d37b3
Compare
the integration test works fine for me... reviewing again now. but looks good |
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 @erikgb for this contribution... it is really great! After (needed) updates to the CRDs etc... this works great. On M1+ k8s 1.24+ is needed... making the 1.25.0 a great starting point. I've added a comment to address but will merge with your attribution and mod tomorrow. I wanted to make sure you had a day to respond (which given the holiday season may be somewhat short response time).
Makefile
Outdated
|
||
# Run e2e tests | ||
.PHONY: e2e-test | ||
e2e-test: cli | ||
./hack/run-e2e-tests.sh | ||
e2e-test: cli envtest |
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 e2e-test
is not a rerun of the integration tests... I think there was some confusion around this. The e2e tests should NOT be dependent on the envtest
. e2e requires docker + kind. Although the refactor of the circleci -> git action lost that.... which is why we continue to have a circleci task along with the github actions. I'm not sure we need to solve all of that in this PR but we should revert this portion. I will wait a day for a response... if none, I will merge and mod, then create a PR for the e2e to be on github actions (assuming it's possible... It will be an area I will need to research).
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've removed the e2e-test
make target.
well... It looks like unintentionally mislead... I'm not sure when the e2e changed. it is using envtest but in a dockerized env. that doesn't seem necessary and we do want a test env with kind for a real e2e. I can see why there was confusion. At this point we should remove the e2e and create a todo to add in a real e2e. I have to assume that we had the real e2e on the kudo side... it seems best to disconnect from kudo and create our own e2e. more clean up to do, but will result in a cleaner project. Please remove the e2e target... but as previously mentioned... I'm happy to do that. |
16d37b3
to
c24872d
Compare
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
c24872d
to
6727eaa
Compare
@kensipe I've removed the e2e-test make target, but unsure where to put the TODO for migrating the e2e tests to real e2e tests using kind. Please advice! |
kind of you sir! I will create a github issue to capture it and schedule into a milestone |
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
What this PR does / why we need it:
This replaces the shell script installation of
etcd
andkube-apiserver
with setup-envtest.Also cleaning out some docs that are outdated by this change.
Upgrading to K8s version
1.25.0
, which is the latest release of kubebuilder-tools.Fixes #428