✨ Add Kubernetes Gomega extension with to make testing controllers easier #1767

schrej · 2022-01-06T23:57:24Z

This is an attempt to revive #1364 by @JoelSpeed. It's essentially the same with a few changes:

I've added the EqualObject matcher from kubernetes-sigs/cluster-api (written by @sbueringer and @killianmuldoon - I hope you're fine with that!)
Added package global functions so it can be used by importing . sigs.k8s.io/controller-runtime/pkg/envtest/komega and use it similar to Gomega
renamed Matcher to komega since it's actually not a Gomega Matcher
changed the With... functions to copy komega

This still needs a lot of tests and documentation.

cc @Danil-Grigorev @joelanford @fabriziopandini since you were interested in the original PR

linux-foundation-easycla · 2022-01-06T23:57:28Z

The committers are authorized under a signed CLA.

✅ Jakob (e09d1d3)

k8s-ci-robot · 2022-01-06T23:57:32Z

Welcome @schrej!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-01-06T23:57:32Z

Hi @schrej. 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.

schrej · 2022-01-07T08:41:20Z

/ok-to-test

fabriziopandini

@schrej thanks for reviving this effort.

While I get EqualObject and HaveField, and I would really like to have them merged, I'm still grokking the Komega client.
If others feel the same, might be breaking this in separated PR could help...

pkg/envtest/komega/interfaces.go

go.mod

pkg/envtest/komega/komega.go

pkg/envtest/komega/interfaces.go

JoelSpeed

In the interest of keeping the review surface small, I'm wondering if we should break this PR up a bit. The addition of the EqualObject matcher makes this PR quite a lot bigger and, as it is code that is already reviewed and accepted within CAPI, maybe should be reviewed separately if we intend to transfer it to this repo. That way we can focus the discussion on the design of the gomega extension and other new code within the PR? WDYT?

pkg/envtest/komega/interfaces.go

pkg/envtest/komega/transforms.go

JoelSpeed · 2022-01-10T13:43:19Z

pkg/envtest/komega/default.go

+// Default is the Komega used by the package global functions.
+// NOTE: Replace it with a custom instance or use WithDefaultClient.
+//       Otherwise the package global functions will panic.
+var Default Komega = &komega{}


Is the intention here that you can import this and use it in the same way that you would with the default Gomega? Rather than having to have the Komega in scope, you just komega.CreateObject().Should(Succeed())?

Only nit is that these names don't match the interface at the moment, would it be good to be consistent?

The intention for the different names was to be a bit more clear that it's about Kubernetes objects when there is no komega in front of it. Also there would be collision with Eventually and Consistently.

Naming isn't great yet though.

Ok that makes sense, just one nit, I don't think it needs to be exported does it? Looking at the usage in this file we can probably make it private

We can, and that even reduces friction with importing both gomega and komega as ..

schrej · 2022-01-13T18:47:24Z

I've removed the matcher as requested. I'll open another PR once this is in, so it's not as confusing.

I've left HaveField() in for now as it's much easier to use when only interested in single properties of an object. Maybe we can extend the EqualObject() matcher to allow loosly matching only the fields that are set on the object passed to EqualObject() (by deleting all patch entries that have nil as their value).

JoelSpeed · 2022-01-25T10:59:21Z

pkg/envtest/komega/default.go

+// Default is the Komega used by the package global functions.
+// NOTE: Replace it with a custom instance or use WithDefaultClient.
+//       Otherwise the package global functions will panic.
+var Default Komega = &komega{}


Ok that makes sense, just one nit, I don't think it needs to be exported does it? Looking at the usage in this file we can probably make it private

pkg/envtest/komega/komega.go

pkg/envtest/komega/transforms.go

schrej · 2022-01-27T10:37:28Z

is pkg/envtest/komega a good location for that package? Or should we maybe move it to pkg/client/komega, since it's heavily relying on that one, or just to pkg/komega directly?

JoelSpeed

Apart from the HaveField naming conflict, I think this is looking pretty good

schrej · 2022-01-28T08:42:55Z

is pkg/envtest/komega a good location for that package? Or should we maybe move it to pkg/client/komega, since it's heavily relying on that one, or just to pkg/komega directly?

@fabriziopandini @vincepri do you have an opinion on that?

JoelSpeed · 2022-01-28T09:20:12Z

/lgtm

I don't have a strong opinion on the naming, but the Komega is looking really clean now and with the gomega HaveField should be really useful for trimming the fat in controller tests

schrej · 2022-03-03T19:52:45Z

After further investigation I don't think it makes sense to add generics at this time. As Stefan pointed out it will obviously require go 1.18, and force all depending projects to use 1.18 as well. Since methods don't support type parameters (only structs), it's also hard to implement it in a beneficial way.
It's only feasible on the package functions that use the default instance.
But using it is pretty nice.

schrej · 2022-03-03T20:26:35Z

Ok. All comments should be addressed now. No generics.
I've added an OWNERS file containing @sbueringer, @JoelSpeed and myself in addition to the regular reviewers and approvers. Is that ok @alvaroaleman?

schrej · 2022-03-03T20:31:43Z

Not related to the changes.
/retest

alvaroaleman · 2022-03-03T20:48:05Z

/approve

alvaroaleman · 2022-03-03T21:08:45Z

Leaving lgtm for one of the new approvers for this

schrej · 2022-03-04T11:58:22Z

/retest

JoelSpeed

Couple of nits otherwise LGTM

pkg/envtest/komega/OWNERS

pkg/envtest/komega/default.go

The helpers can be used to write tests using the gomega library in conjunction with the Kubernetes API, or a mock of it. Co-Authored-By: Joel Speed <joel.speed@hotmail.co.uk>

JoelSpeed

/lgtm

Thanks for all your work on this @schrej

k8s-ci-robot · 2022-03-04T12:46:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, JoelSpeed, schrej

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~~ [alvaroaleman]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 6, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 6, 2022

k8s-ci-robot requested review from mengqiy and varshaprasad96 January 6, 2022 23:57

schrej force-pushed the feature/komega branch from dad1f15 to 87675c9 Compare January 7, 2022 00:11

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 Jan 7, 2022

fabriziopandini reviewed Jan 8, 2022

View reviewed changes

pkg/envtest/komega/interfaces.go Show resolved Hide resolved

killianmuldoon mentioned this pull request Jan 10, 2022

Integrate internal/matchers into Komega style test abstraction in Controller Runtime kubernetes-sigs/cluster-api#5338

Closed

sbueringer reviewed Jan 10, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

pkg/envtest/komega/komega.go Outdated Show resolved Hide resolved

pkg/envtest/komega/komega.go Outdated Show resolved Hide resolved

pkg/envtest/komega/interfaces.go Show resolved Hide resolved

JoelSpeed reviewed Jan 10, 2022

View reviewed changes

pkg/envtest/komega/interfaces.go Show resolved Hide resolved

JoelSpeed reviewed Jan 10, 2022

View reviewed changes

schrej force-pushed the feature/komega branch from 87675c9 to be1fcf6 Compare January 13, 2022 18:33

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 13, 2022

schrej force-pushed the feature/komega branch from be1fcf6 to 33e6134 Compare January 24, 2022 19:06

JoelSpeed reviewed Jan 25, 2022

View reviewed changes

schrej force-pushed the feature/komega branch from 33e6134 to ec47819 Compare January 27, 2022 16:12

JoelSpeed reviewed Jan 27, 2022

View reviewed changes

schrej force-pushed the feature/komega branch 2 times, most recently from ab25b63 to bf5d641 Compare January 28, 2022 08:38

schrej force-pushed the feature/komega branch from bf5d641 to e09d1d3 Compare March 3, 2022 20:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2022

schrej force-pushed the feature/komega branch from e09d1d3 to 5098ac9 Compare March 3, 2022 20:21

k8s-ci-robot added do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 3, 2022

schrej force-pushed the feature/komega branch from 5098ac9 to a29bda3 Compare March 3, 2022 20:23

k8s-ci-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Mar 3, 2022

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

schrej force-pushed the feature/komega branch 2 times, most recently from f772b9d to 9b054aa Compare March 4, 2022 11:49

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 4, 2022

JoelSpeed reviewed Mar 4, 2022

View reviewed changes

pkg/envtest/komega/OWNERS Outdated Show resolved Hide resolved

pkg/envtest/komega/default.go Show resolved Hide resolved

schrej force-pushed the feature/komega branch 2 times, most recently from 0dc4549 to 3d4dfd2 Compare March 4, 2022 12:42

Add komega package with helpers for writing tests

c19a6ed

The helpers can be used to write tests using the gomega library in conjunction with the Kubernetes API, or a mock of it. Co-Authored-By: Joel Speed <joel.speed@hotmail.co.uk>

schrej force-pushed the feature/komega branch from 3d4dfd2 to c19a6ed Compare March 4, 2022 12:45

JoelSpeed approved these changes Mar 4, 2022

View reviewed changes

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

k8s-ci-robot merged commit 9ee63fc into kubernetes-sigs:master Mar 4, 2022

k8s-ci-robot added this to the v0.10.x milestone Mar 4, 2022

schrej deleted the feature/komega branch March 4, 2022 12:52

rfranzke mentioned this pull request May 17, 2022

Upgrade k8s.io/* to v0.24, sigs.k8s.io/controller-runtime to v0.12.1+ gardener/gardener#5990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add Kubernetes Gomega extension with to make testing controllers easier #1767

✨ Add Kubernetes Gomega extension with to make testing controllers easier #1767

schrej commented Jan 6, 2022 •

edited

linux-foundation-easycla bot commented Jan 6, 2022 •

edited

k8s-ci-robot commented Jan 6, 2022

k8s-ci-robot commented Jan 6, 2022

schrej commented Jan 7, 2022

fabriziopandini left a comment

JoelSpeed left a comment

JoelSpeed Jan 10, 2022

schrej Jan 10, 2022

JoelSpeed Jan 25, 2022

schrej Jan 27, 2022

schrej commented Jan 13, 2022

JoelSpeed Jan 25, 2022

schrej commented Jan 27, 2022

JoelSpeed left a comment

schrej commented Jan 28, 2022

JoelSpeed commented Jan 28, 2022

schrej commented Mar 3, 2022

schrej commented Mar 3, 2022

schrej commented Mar 3, 2022

alvaroaleman commented Mar 3, 2022

alvaroaleman commented Mar 3, 2022

schrej commented Mar 4, 2022

JoelSpeed left a comment

JoelSpeed left a comment

k8s-ci-robot commented Mar 4, 2022

✨ Add Kubernetes Gomega extension with to make testing controllers easier #1767

✨ Add Kubernetes Gomega extension with to make testing controllers easier #1767

Conversation

schrej commented Jan 6, 2022 • edited

linux-foundation-easycla bot commented Jan 6, 2022 • edited

k8s-ci-robot commented Jan 6, 2022

k8s-ci-robot commented Jan 6, 2022

schrej commented Jan 7, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Jan 10, 2022

Choose a reason for hiding this comment

schrej Jan 10, 2022

Choose a reason for hiding this comment

JoelSpeed Jan 25, 2022

Choose a reason for hiding this comment

schrej Jan 27, 2022

Choose a reason for hiding this comment

schrej commented Jan 13, 2022

JoelSpeed Jan 25, 2022

Choose a reason for hiding this comment

schrej commented Jan 27, 2022

JoelSpeed left a comment

Choose a reason for hiding this comment

schrej commented Jan 28, 2022

JoelSpeed commented Jan 28, 2022

schrej commented Mar 3, 2022

schrej commented Mar 3, 2022

schrej commented Mar 3, 2022

alvaroaleman commented Mar 3, 2022

alvaroaleman commented Mar 3, 2022

schrej commented Mar 4, 2022

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 4, 2022

schrej commented Jan 6, 2022 •

edited

linux-foundation-easycla bot commented Jan 6, 2022 •

edited