🏃 Add tests for cluster controller #1199

lentzi90 · 2022-04-13T09:16:00Z

What this PR does / why we need it:

This adds some simple tests for the OpenStackCluster controller.

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Note that there are 3 commit. I can make separate PRs for them or remove some of them if needed.
This file was copied almost verbatim from CAPA. We could alternatively import the relevant package. What do you think would be better?
Please note the pending test. I'm not sure how to move forward with this and would appreciate any hints. If there are no obvious solutions I think maybe we should have an issue to discuss this further?

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

netlify · 2022-04-13T09:16:04Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`65e0c82`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/627a1969806b9100095a88fd
😎 Deploy Preview	https://deploy-preview-1199--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2022-04-13T09:16:09Z

Hi @lentzi90. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

jichenjc · 2022-04-14T01:40:51Z

/ok-to-test

lentzi90 · 2022-04-28T10:16:38Z

/test pull-cluster-api-provider-openstack-test

lentzi90 · 2022-04-28T10:36:16Z

/test pull-cluster-api-provider-openstack-test

lentzi90 · 2022-04-29T05:49:30Z

/test pull-cluster-api-provider-openstack-test

lentzi90 · 2022-04-29T06:17:35Z

/test pull-cluster-api-provider-openstack-test

lentzi90 · 2022-04-29T06:24:54Z

/test pull-cluster-api-provider-openstack-build
/test pull-cluster-api-provider-openstack-e2e-test

lentzi90 · 2022-04-29T07:31:22Z

/hold cancel

lentzi90 · 2022-04-29T10:42:19Z

Alright I think this is ready for review now. Note that there are 3 commits. This is to make it easy to remove some of them if we decide to not take them in. Alternatively I can make separate PRs also for each.
I'll add a few things to note in the description also.

lentzi90 · 2022-04-29T10:49:36Z

controllers/openstackcluster_controller_test.go

+	})
+
+	// TODO: This test is set to pending (PIt instead of It) since it is not working.
+	PIt("should be able to reconcile when basition disabled", func() {


Please take a look at this test specifically. I'm not sure how to move forward here. The basic tests above does not require any interaction with the Openstack API, but this one does and I didn't find a good way to fake it. Am I missing something? Is it possible to get a fake/mock scope or dummy openstack API to use in tests like these?

apricote · 2022-04-29T11:13:23Z

controllers/suite_test.go

@@ -57,6 +59,11 @@ var _ = BeforeSuite(func() {
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "config", "crd", "bases"),
 		},
+		// Add fake CAPI CRDs that we reference
+		CRDs: []*apiextensionsv1.CustomResourceDefinition{
+			external.TestClusterCRD.DeepCopy(),


What is the benefit of faking the CRDs instead of including the actual ones? (I have not really worked with envtest before)

I think the main benefit is that we don't have to include the CAPI CRDs either in the repo or automatically download them to be able to run the tests.

Ah right, because this expects CustomResourceDefinition objects, but we currently only import the API types (from sigs.k8s.io/cluster-api/api/*.

That makes sense. Thank you :)

You could also make use of "sigs.k8s.io/cluster-api/internal/test/envtest" if it makes sense. It also includes some helper ufnctions which may be handy.

lentzi90 · 2022-05-09T12:18:51Z

I have split this up into two now. The first part is #1239 and this is the second. I'll rebase this once the first is merged.

/hold

In order to use envtest for these tests, it was necessary to add the CRDs for CAPI clusters and machines. Luckily I found a way to only include the minimal required parts instead of the full thing. This solution is taken from CAPA.

lentzi90 · 2022-05-10T08:03:53Z

@mdbooth this is now rebased and ready to go in I think. The only thing I'm wondering is if I should remove the disabled test for now or keep it there as a TODO. What do you think?

mdbooth · 2022-05-10T10:36:14Z

/approve

k8s-ci-robot · 2022-05-10T10:36:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90, 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:

~~OWNERS~~ [mdbooth]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mdbooth · 2022-05-10T10:36:56Z

@mdbooth this is now rebased and ready to go in I think. The only thing I'm wondering is if I should remove the disabled test for now or keep it there as a TODO. What do you think?

I don't think it's important. It's a little messy but it's documented why it's messy and we have a plan to fix it up soon.

apricote · 2022-05-10T12:25:22Z

/lgtm

lentzi90 · 2022-05-10T12:31:56Z

/hold cancel

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 13, 2022

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2022

k8s-ci-robot requested review from chrischdi and prankul88 April 13, 2022 09:16

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 13, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 14, 2022

lentzi90 force-pushed the lentzi90/cluster-controller-tests branch from adcb133 to 8a6c46f Compare April 28, 2022 10:13

lentzi90 force-pushed the lentzi90/cluster-controller-tests branch from 8a6c46f to b967fd7 Compare April 28, 2022 10:35

lentzi90 force-pushed the lentzi90/cluster-controller-tests branch from b967fd7 to 3c516e3 Compare April 29, 2022 05:48

lentzi90 force-pushed the lentzi90/cluster-controller-tests branch from 3c516e3 to 4a6cbfe Compare April 29, 2022 06:16

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2022

lentzi90 marked this pull request as ready for review April 29, 2022 07:31

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2022

k8s-ci-robot requested review from jichenjc and seanschneeweiss April 29, 2022 07:31

lentzi90 commented Apr 29, 2022

View reviewed changes

apricote reviewed Apr 29, 2022

View reviewed changes

mdbooth mentioned this pull request Apr 29, 2022

Clusters without bastions do not reconcile #1230

Closed

mdbooth mentioned this pull request May 6, 2022

Envtest mocks #1236

Merged

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 9, 2022

lentzi90 force-pushed the lentzi90/cluster-controller-tests branch from 4a6cbfe to 65e0c82 Compare May 10, 2022 07:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2022

Openstackcluster controller: Add some basic tests

65e0c82

In order to use envtest for these tests, it was necessary to add the CRDs for CAPI clusters and machines. Luckily I found a way to only include the minimal required parts instead of the full thing. This solution is taken from CAPA.

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2022

k8s-ci-robot assigned apricote May 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2022

k8s-ci-robot merged commit c362ac7 into kubernetes-sigs:main May 10, 2022

lentzi90 deleted the lentzi90/cluster-controller-tests branch May 11, 2022 06:02

lentzi90 mentioned this pull request Oct 28, 2022

REQUEST: New membership for @lentzi90 kubernetes/org#3793

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏃 Add tests for cluster controller #1199

🏃 Add tests for cluster controller #1199

lentzi90 commented Apr 13, 2022 •

edited

Loading

netlify bot commented Apr 13, 2022 •

edited

Loading

k8s-ci-robot commented Apr 13, 2022

jichenjc commented Apr 14, 2022

lentzi90 commented Apr 28, 2022

lentzi90 commented Apr 28, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 Apr 29, 2022

apricote Apr 29, 2022

lentzi90 Apr 29, 2022

apricote Apr 29, 2022

chrischdi May 9, 2022

lentzi90 commented May 9, 2022

lentzi90 commented May 10, 2022

mdbooth commented May 10, 2022

k8s-ci-robot commented May 10, 2022

mdbooth commented May 10, 2022

apricote commented May 10, 2022

lentzi90 commented May 10, 2022

🏃 Add tests for cluster controller #1199

🏃 Add tests for cluster controller #1199

Conversation

lentzi90 commented Apr 13, 2022 • edited Loading

netlify bot commented Apr 13, 2022 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

k8s-ci-robot commented Apr 13, 2022

jichenjc commented Apr 14, 2022

lentzi90 commented Apr 28, 2022

lentzi90 commented Apr 28, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 commented Apr 29, 2022

lentzi90 Apr 29, 2022

Choose a reason for hiding this comment

apricote Apr 29, 2022

Choose a reason for hiding this comment

lentzi90 Apr 29, 2022

Choose a reason for hiding this comment

apricote Apr 29, 2022

Choose a reason for hiding this comment

chrischdi May 9, 2022

Choose a reason for hiding this comment

lentzi90 commented May 9, 2022

lentzi90 commented May 10, 2022

mdbooth commented May 10, 2022

k8s-ci-robot commented May 10, 2022

mdbooth commented May 10, 2022

apricote commented May 10, 2022

lentzi90 commented May 10, 2022

lentzi90 commented Apr 13, 2022 •

edited

Loading

netlify bot commented Apr 13, 2022 •

edited

Loading