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.

Sign up for GitHub

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

Jump to bottom

✨ Added methods to intercept calls to a k8s client #2248

Merged
merged 1 commit into from
Apr 25, 2023
Merged

✨ Added methods to intercept calls to a k8s client #2248

merged 1 commit into from
Apr 25, 2023

Conversation

ludydoo
Copy link
Contributor

@ludydoo ludydoo commented Mar 27, 2023 •
edited
Loading

This PR adds some methods to allow consumers of the k8s client to intercept kubernetes api calls to inject custom logic, which is helpful while testing e.g. expected behavior upon certain types of failures.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @ludydoo!

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 k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 27, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @ludydoo. 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 the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 27, 2023
@ludydoo ludydoo mentioned this pull request Mar 27, 2023
Merged
@alvaroaleman
Copy link
Member

/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 Mar 27, 2023
@alvaroaleman alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 27, 2023
@ludydoo
Copy link
Contributor Author

ludydoo commented Mar 27, 2023

/retest

@alvaroaleman
Copy link
Member

I like it, simple, stupid and maximum flexibility. Thoughts @joelanford @vincepri @sbueringer ?

@ludydoo
Copy link
Contributor Author

ludydoo commented Mar 28, 2023 •
edited
Loading

Hi @alvaroaleman , thanks a lot for your quick review! What's your opinion about: would it make more sense to put this into the client package? Personally I don't see much use-case for a non-testing scenario, so the fake package makes sense, but in terms of usage/readability/descriptive naming, perhaps it's clearer if consumers will use some client.Interceptor rather than some fake.Interceptor ?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 28, 2023
@ludydoo ludydoo requested review from inteon and removed request for joelanford and alvaroaleman March 28, 2023 10:46
@ludydoo
Copy link
Contributor Author

ludydoo commented Mar 29, 2023

@alvaroaleman @inteon please let me know if there is anything I should tweak or modify

@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 29, 2023
@alvaroaleman
Copy link
Member

@ludydoo we had a lot of discussion about fakeclient changes like this, which is why I can't just merge without some of the other maintainers approving and two of them are on PTO unfortunately. So this will take a bit of time, but there is nothing expected from you at this point. Sorry about that.

vincepri
vincepri reviewed Apr 10, 2023
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

This looks like a good way to allow some mocking to happen in the fake client, I'm personally +1, but would like to hear @sbueringer's opinion as well.

@alvaroaleman alvaroaleman mentioned this pull request Apr 10, 2023
Closed
@sbueringer
Copy link
Member

Just back from PTO, will take a look ASAP, not sure if it will be before KubeCon

@vincepri vincepri added this to the v0.15.x milestone Apr 12, 2023
@sbueringer
Copy link
Member

sbueringer commented Apr 14, 2023 •
edited
Loading

Looks good to me as well. Looks like it needs a rebase to pick up the change to the golangci-lint action.

One thought (similar to: #2248 (comment)): as this functionality is probably often used together with fake client, but it can also be used with a regular client, should we move it out of the fake package?

I was thinking about maybe a new package like pkg/client/interceptor. Has the benefit to keep it clearly separate from the fake client and if we rename the func we could make it e.g. interceptor.NewClient instead of fake.Interceptor.

@alvaroaleman
Copy link
Member

If we move this into a distinct package, can we make this discoverable by integrating with the fake client builder by adding a With method for each of the interceptor methods that sets them?

@ludydoo
Copy link
Contributor Author

ludydoo commented Apr 16, 2023 •
edited
Loading

If we move this into a distinct package, can we make this discoverable by integrating with the fake client builder by adding a With method for each of the interceptor methods that sets them?

@alvaroaleman @sbueringer

I'm not sure how to resolve the circular dependency in this case, since the interceptor_test import the fake package.
Should I put the test in a subpackage of interceptor ?

edit

... or simply remove the dependency on fake and implement a dummy client.

I've moved the interception to the pkg/client/interceptor package, and added a method to configure the fake.ClientBuilder to optionally use interception.

@ludydoo ludydoo changed the title ✨ Added methods to intercept calls to a fake k8s client ✨ Added methods to intercept calls to a k8s client Apr 16, 2023
@ludydoo
Copy link
Contributor Author

ludydoo commented Apr 16, 2023

/retest

@ludydoo
Copy link
Contributor Author

ludydoo commented Apr 20, 2023

@alvaroaleman @sbueringer just pinging you for feedback on the changes when you have some time

sbueringer
sbueringer reviewed Apr 24, 2023
View reviewed changes
pkg/client/interceptor/intercept.go Outdated Show resolved Hide resolved
pkg/client/interceptor/intercept.go Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member

sbueringer commented Apr 24, 2023 •
edited
Loading

@ludydoo Can you please drop the merge commits, squash and rebase to main?

(I assume you probably kept them for easier reviews, but the delta of this PR would be easier to see without the merge commits)

@ludydoo
Squashed commit of the following:
c143c83 
commit 3b534a4
Merge: f264518 a24b949
Author: Ludovic Cleroux <[email protected]>
Date:   Tue Apr 25 12:58:42 2023 +0200

    Merge branch 'kubernetes-sigs:main' into add-fake-client-interception

commit f264518
Author: Ludovic Cleroux <[email protected]>
Date:   Tue Apr 25 12:57:18 2023 +0200

    PR comments

commit e358be3
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 10:38:37 2023 +0200

    Allow the interceptor to be discovered from the fake ClientBuilder

commit bf25854
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 10:21:37 2023 +0200

    Remove dependency on fake client

commit 6063435
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 10:02:42 2023 +0200

    Cleanup

commit b66b57a
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 10:02:05 2023 +0200

    Cleanup

commit 483deea
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 10:00:39 2023 +0200

    Cleanup

commit 9c3df6a
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 09:59:19 2023 +0200

    Slight refactor

commit 4fb67c0
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 09:56:24 2023 +0200

    Slight refactor & lint

commit 5221233
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 09:30:52 2023 +0200

    Move interceptor to new package

commit d74b869
Merge: 9642a63 d989e66
Author: Ludovic Cleroux <[email protected]>
Date:   Sun Apr 16 09:21:31 2023 +0200

    Merge branch 'kubernetes-sigs:main' into add-fake-client-interception

commit 9642a63
Author: Ludovic Cleroux <[email protected]>
Date:   Wed Mar 29 09:34:28 2023 +0200

    Fixed SubResource/Status calling logic

commit 00597ac
Author: Ludovic Cleroux <[email protected]>
Date:   Tue Mar 28 09:44:36 2023 +0200

    Remove unnecessary Status() intercept

commit cd8451c
Author: Ludovic Cleroux <[email protected]>
Date:   Mon Mar 27 15:52:40 2023 +0200

    Fix goimports

commit c80013e
Author: Ludovic Cleroux <[email protected]>
Date:   Mon Mar 27 15:42:45 2023 +0200

    Fix missing comments

commit fe27862
Author: Ludovic Cleroux <[email protected]>
Date:   Mon Mar 27 15:30:13 2023 +0200

    Added methods to intercept calls to a fake k8s client
@ludydoo ludydoo force-pushed the add-fake-client-interception branch from 3b534a4 to c143c83 Compare April 25, 2023 11:01
@ludydoo
Copy link
Contributor Author

ludydoo commented Apr 25, 2023

@ludydoo Can you please drop the merge commits, squash and rebase to main?

(I assume you probably kept them for easier reviews, but the delta of this PR would be easier to see without the merge commits)

@sbueringer just addressed your comments, and squashed & rebased to main. Please let me know if you think there should be some more changes.

@ludydoo ludydoo requested a review from sbueringer April 25, 2023 13:28
@sbueringer
Copy link
Member

@ludydoo Thank you very much!!

/lgtm

/assign @vincepri @alvaroaleman

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2023
alvaroaleman
alvaroaleman approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

Thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, ludydoo

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0f5aba9 into kubernetes-sigs:main Apr 25, 2023
@alvaroaleman
Copy link
Member

Thanks a lot for this @ludydoo ! This makes injecting custom beheavior into the fakeclient for tests much easier :)

@ludydoo
Copy link
Contributor Author

ludydoo commented Apr 25, 2023

Thank you for your reviews and advices @alvaroaleman @sbueringer @inteon @vincepri !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@inteon inteon Awaiting requested review from inteon

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

@vincepri vincepri

@sbueringer sbueringer

@alvaroaleman alvaroaleman

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 "Looks good to me", 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v0.15.x
Development

Successfully merging this pull request may close these issues.
6 participants
@ludydoo @k8s-ci-robot @alvaroaleman @sbueringer @vincepri @inteon