-
Notifications
You must be signed in to change notification settings - Fork 31
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
e2e testing for CAPI with CAPO #280
Conversation
EmilienM
commented
Nov 23, 2023
- Add cluster-capi-operator integration
- vendoring: add cluster-capi-operator && go mod vendor
369122c
to
344a698
Compare
344a698
to
6fcd6de
Compare
6fcd6de
to
0c79afc
Compare
041cfb5
to
6abc212
Compare
6abc212
to
bfb88a1
Compare
These are heavily based on the tests for other platforms, which are currently included in the cluster-capi-operator tree [1] but which will eventually be moved out to the openshift forks of the respective CAPI implementations. The key difference from these is that (a) we don't create a cluster (since we have the infracluster controller for this) and (b) we obviously use OpenStack-specific semantics. [1] https://github.com/openshift/cluster-capi-operator/tree/release-4.15/e2e Co-Authored-By: Emilien Macchi <emilien@redhat.com> Co-Authored-By: Stephen Finucane <stephenfin@redhat.com>
bfb88a1
to
07700ae
Compare
/test e2e-techpreview |
@stephenfin I think something is failing on purpose, due to the fact the job is TechPreview:
i'll investigate that |
In fact I think the error is really:
|
/test e2e-techpreview |
/lgtm |
@EmilienM: you cannot LGTM your own PR. In response to this:
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilienM 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 |
k8s.io/api v0.28.2 | ||
k8s.io/apimachinery v0.28.2 | ||
k8s.io/client-go v0.28.1 | ||
sigs.k8s.io/cluster-api v1.5.2 |
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.
This makes me slightly nervous, because it highlights a diamond dependency that we have.
i.e. CAPO depends on CAPI version X. cluster-capi-operator depends on CAPI version Y. If CAPO depends on cluster-capi-operator, what happens when X and Y are not reconcilable?
Would it be feasible to define the e2e tests in their own go module and remove all dependencies on CAPO/openshift? It took a quick look, and I think the e2e tests may only be importing some constants? If we duplicated those constants in the test, which may not be a bad idea anyway, can we eliminate the dependency?
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.
Yeah, it looks like we're only using the constants so I can drop those.
I'm going to approve this as-is. The diamond dependency is real, but we can address with a refactor when it comes up as an issue. /lgtm |
/test e2e-techpreview Actual test passed. Failure came later during the gather. |
@EmilienM: all tests passed! Full PR test history. Your PR dashboard. 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. |
[ART PR BUILD NOTIFIER] This PR has been included in build openstack-cluster-api-controllers-container-v4.16.0-202312130634.p0.gdb44aa6.assembly.stream for distgit openstack-cluster-api-controllers. |