✨ Add API validation test framework #1919
Conversation
TEST_PATHS defaults to the previous hardcoded value of './...', but can now be overridden. e.g. To run just the controller tests: make test TEST_PATHS=./controllers/...
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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-ci-robot
added
the
approved
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
size/L
|
/cc @JoelSpeed whose CPMS code I cribbed from heavily. |
| Expect((&infrav1.OpenStackMachineTemplateWebhook{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachineTemplate webhook should be registered with manager") | ||
| Expect((&infrav1.OpenStackMachineTemplateList{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachineTemplateList webhook should be registered with manager") | ||
| Expect((&infrav1.OpenStackCluster{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackCluster webhook should be registered with manager") | ||
| Expect((&infrav1.OpenStackClusterTemplate{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackClusterTemplate webhook should be registered with manager") | ||
| Expect((&infrav1.OpenStackMachine{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachine webhook should be registered with manager") | ||
| Expect((&infrav1.OpenStackMachineList{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachineList webhook should be registered with manager") | ||
| Expect((&infrav1.OpenStackClusterList{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackClusterList webhook should be registered with manager") |
There was a problem hiding this comment.
In general it's bad practice to have code attached to your API types, introduces dependencies and makes it hard for people to import your API types. We've resolved this issue in core CAPI, will want to resolve in wider ecosystem too
There was a problem hiding this comment.
Yep. We've had a user report about that, too. It's on the TODO list!
There was a problem hiding this comment.
For reference: #1891
There was a problem hiding this comment.
@JoelSpeed Just for you: #1920
|
|
||
| minorInt, err := strconv.Atoi(serverVersion.Minor) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(minorInt).To(BeNumerically(">=", 25), fmt.Sprintf("This test suite requires a Kube API server of at least version 1.25, current version is 1.%s", serverVersion.Minor)) |
There was a problem hiding this comment.
Depending on which validations you are using, this number may be greater. What is the minimum supported version for running this controller?
There was a problem hiding this comment.
Right now we're not relying any anything new. We don't even require CEL support. However, I deliberate left this in because:
- 1.25 is probably a reasonable support minimum right now
- You keep badgering me to add CEL support, and I'm hoping to get to it soon
mdbooth
force-pushed
the
apivalidationtests
branch
4 times, most recently
from
23ab488
to
a6f2a46
Compare
k8s-ci-robot
added
size/XL
|
/lgtm add a hold in case anyone want to see this again |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
mdbooth
force-pushed
the
apivalidationtests
branch
from
a6f2a46
to
4d3e79c
Compare
k8s-ci-robot
removed
the
lgtm
|
Interesting that it didn't re-run the tests 🤔 /test all |
|
/test pull-cluster-api-provider-openstack-test |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
This series adds a framework for testing API validations via calls to a running apiserver using envtest. It initially adds only a couple of example tests.
Firstly we update ginkgo to have the same version in tools and the main module, because otherwise it emits a warning.
The we fix an actual bug discovered by the example test:
managedSecurityGroupswas missing anomitempty, which caused creation of a bare cluster object to fail. We will likely want to visit the minimum set of required parameters, but this was an obvious bug.Running the tests requires a set of assets to have been downloaded, which the
make testtarget conveniently does for us. However, it wasn't possible to restrictmake testto only the tests we are interested in, so we give it an optionalTEST_PATHSparameters.Finally we add a new test suite with 2 trivial tests, enough to demonstrate that it is working correctly.