Skip to content
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

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

Merged
merged 1 commit into from Mar 4, 2022

Conversation

schrej
Copy link
Member

@schrej schrej commented Jan 6, 2022

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
Copy link

linux-foundation-easycla bot commented Jan 6, 2022

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot 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
Copy link
Contributor

k8s-ci-robot commented Jan 6, 2022

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
Copy link
Contributor

k8s-ci-robot commented Jan 6, 2022

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.

@k8s-ci-robot 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
@schrej
Copy link
Member Author

schrej commented Jan 7, 2022

/ok-to-test

@k8s-ci-robot 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
Copy link
Member

@fabriziopandini fabriziopandini left a comment

@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 Show resolved Hide resolved
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
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

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 Show resolved Hide resolved
pkg/envtest/komega/transforms.go Outdated Show resolved Hide resolved
// 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{}
Copy link
Contributor

@JoelSpeed JoelSpeed Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@schrej schrej Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@JoelSpeed JoelSpeed Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@schrej schrej Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@k8s-ci-robot 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
Copy link
Member Author

schrej commented Jan 13, 2022

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).

// 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{}
Copy link
Contributor

@JoelSpeed JoelSpeed Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
pkg/envtest/komega/transforms.go Outdated Show resolved Hide resolved
@schrej
Copy link
Member Author

schrej commented Jan 27, 2022

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?

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

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

@schrej schrej force-pushed the feature/komega branch 2 times, most recently from ab25b63 to bf5d641 Compare Jan 28, 2022
@schrej
Copy link
Member Author

schrej commented Jan 28, 2022

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
Copy link
Contributor

JoelSpeed commented Jan 28, 2022

/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
Copy link
Member Author

schrej commented Mar 3, 2022

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.

@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2022
@k8s-ci-robot 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
@k8s-ci-robot 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
@schrej
Copy link
Member Author

schrej commented Mar 3, 2022

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
Copy link
Member Author

schrej commented Mar 3, 2022

Not related to the changes.
/retest

@alvaroaleman
Copy link
Member

alvaroaleman commented Mar 3, 2022

/approve

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

alvaroaleman commented Mar 3, 2022

Leaving lgtm for one of the new approvers for this

@schrej schrej force-pushed the feature/komega branch 2 times, most recently from f772b9d to 9b054aa Compare Mar 4, 2022
@k8s-ci-robot 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
@schrej
Copy link
Member Author

schrej commented Mar 4, 2022

/retest

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Couple of nits otherwise LGTM

pkg/envtest/komega/OWNERS Outdated Show resolved Hide resolved
pkg/envtest/komega/default.go Show resolved Hide resolved
@schrej schrej force-pushed the feature/komega branch 2 times, most recently from 0dc4549 to 3d4dfd2 Compare Mar 4, 2022
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>
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

/lgtm

Thanks for all your work on this @schrej

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 4, 2022

[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:

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 k8s-ci-robot merged commit 9ee63fc into kubernetes-sigs:master Mar 4, 2022
7 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.10.x milestone Mar 4, 2022
@schrej schrej deleted the feature/komega branch Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants