✨add field selector enabled fake client #800

answer1991 · 2020-02-15T08:29:10Z

Real API Server client could set up indexer using Manager provided method GetFieldIndexer:

// Setup additional indexer.
	err = mgr.GetFieldIndexer().IndexField(&v1.Pod{}, "spec.nodeName",
		func(o runtime.Object) []string {
			pod, ok := o.(*v1.Pod)
			if !ok {
				return nil
			}
			return []string{pod.Spec.NodeName}
		})

And then we can list objects using field selector:

mgr.GetClient().List(context.TODO(), &v1.PodList{}, client.MatchingFields(fields.Set{"spec.nodeName": "foo"}))

But fake client can not, this PR enable us wrapper fake client and got a field selector enable client.

k8s-ci-robot · 2020-02-15T08:29:18Z

Welcome @answer1991!

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 · 2020-02-15T08:29:18Z

Hi @answer1991. 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 · 2020-02-15T08:29:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: answer1991
To complete the pull request process, please assign gerred
You can assign the PR to them by writing /assign @gerred in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

answer1991 · 2020-02-15T08:33:30Z

Not sure how to print the icon in title :-P

alvaroaleman

Thanks for picking this up!

I wrote a similarish issue a while ago: #657

Generally speaking I think it would be great if we extended the existing fakeClient and allowed to configure indexers on it, rather than adding a new constructor. WDYT?

alvaroaleman · 2020-02-15T08:59:30Z

pkg/client/fake/field_selector_client.go

+var _ client.Client = &fieldSelectorEnableClient{}
+
+// NewFieldSelectorEnableClient wrapper a fake client that List method enables field selector
+func NewFieldSelectorEnableClient(client client.Client, scheme *runtime.Scheme, obj runtime.Object, field string, indexerFunc client.IndexerFunc) (client.Client, error) {


This means we can set up only one indexer, I think it would be better if ppl could set up any number

No, we can set up multi indexes like that:

fakeClient := NewFakeClientWithScheme(scheme.Scheme) fieldSelectorClient, err := NewFieldSelectorEnableClient(fakeClient, scheme.Scheme, &corev1.Pod{}, "spec.nodeName", func(o runtime.Object) []string { po, ok := o.(*corev1.Pod) if !ok { return nil } return []string{po.Spec.NodeName} }) fieldSelectorClient, err = NewFieldSelectorEnableClient(fieldSelectorClient, scheme.Scheme, &corev1.Node{}, "spec.podCIDR", func(o runtime.Object) []string { no, ok := o.(*corev1.Node) if !ok { return nil } return []string{no.Spec.PodCIDR} })

I had already add multi indexer cases in test codes.

alvaroaleman · 2020-02-15T09:00:21Z

pkg/client/fake/field_selector_client.go

+	}
+
+	if gvk, err := apiutil.GVKForObject(objs[0], f.scheme); err != nil {
+		return nil


Why do we swallow this error?

fixed, thanks!

answer1991 · 2020-02-15T09:25:09Z

Generally speaking I think it would be great if we extended the existing fakeClient and allowed to configure indexers on it, rather than adding a new constructor. WDYT?

Because of setting up indexer need lots of parameters, and indexer only required in only some cases or scenes. I think function NewFakeClientWithScheme args is simple and clean, I do not think we should break this func signature, so I think it's better to provider a wrapper func like my PR did.

vincepri · 2020-02-15T15:38:46Z

+1 on extending the existing fake client somehow, rather than creating a separate one. We can probably find ways to make sure it's opt-in, and only needs to be configured when needed.

alvaroaleman · 2020-02-15T16:56:43Z

Because of setting up indexer need lots of parameters, and indexer only required in only some cases or scenes. I think function NewFakeClientWithScheme args is simple and clean, I do not think we should break this func signature, so I think it's better to provider a wrapper func like my PR did

Agreed, we shouldn't extend the signature. Possibilities to allow adding indexers without doing that include:

make NewFakeClientWithScheme not return a client.Client but an interface that embeds both client.Client and client.FieldIndexer
add a AddFieldIndexer(c client.Client, obj runtime.Object, field string, extractValue client.IndexerFunc) that type asserts the client as *fakeClient and panics if that fails

But: Lets give folks time over the next week to express an opinion or ideas before you end up changing it back and forth depending on what the last reviewer thought :)

answer1991 · 2020-02-17T14:42:57Z

add a AddFieldIndexer(c client.Client, obj runtime.Object, field string, extractValue client.IndexerFunc) that type asserts the client as *fakeClient and panics if that fails

The func signature is same with the func my PR NewFieldSelectorEnableClient but only the name is different. Should I just move the implement into fakeClient, then you guys will be LGTM :- )

vincepri · 2020-02-21T18:11:09Z

/ok-to-test
/hold

JoelSpeed · 2020-02-24T16:38:21Z

add a AddFieldIndexer(c client.Client, obj runtime.Object, field string, extractValue client.IndexerFunc) that type asserts the client as *fakeClient and panics if that fails

If this is living within the fake package I think it's a neat way of achieving this without causing issues with interfaces etc elsewhere.

My concern with the other suggestion is that if people declare their variables ahead of time as var c client.Client and then populate it at another point, they would then have to do some type assertions elsewhere to get the client.FieldIndexer methods available

fejta-bot · 2020-05-24T16:46:25Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

vincepri · 2020-06-08T16:06:23Z

Having

make NewFakeClientWithScheme not return a client.Client but an interface that embeds both client.Client and client.FieldIndexer

with a super-interface should be backward compatible (apart from the function signature)

vincepri · 2020-06-08T16:06:31Z

/remove-lifecycle stale

fejta-bot · 2020-09-06T16:39:36Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-10-06T17:21:52Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-11-05T18:04:18Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-11-05T18:04:26Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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 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 Feb 15, 2020

k8s-ci-robot requested review from DirectXMan12 and vincepri February 15, 2020 08:29

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2020

answer1991 changed the title ~~✨ add field selector enabled fake client~~ :sparkles: add field selector enabled fake client Feb 15, 2020

answer1991 changed the title ~~:sparkles: add field selector enabled fake client~~ ✨ add field selector enabled fake client Feb 15, 2020

answer1991 changed the title ~~✨ add field selector enabled fake client~~ ✨ add field selector enabled fake client Feb 15, 2020

alvaroaleman reviewed Feb 15, 2020

View reviewed changes

answer1991 force-pushed the feature/fake-client-enable-field-selector branch from bba3bdd to 0b410af Compare February 15, 2020 09:18

answer1991 force-pushed the feature/fake-client-enable-field-selector branch from 0b410af to d7e5b8e Compare February 15, 2020 09:52

add field selector enabled fake client

b5f85d2

answer1991 force-pushed the feature/fake-client-enable-field-selector branch from d7e5b8e to b5f85d2 Compare February 15, 2020 12:02

answer1991 changed the title ~~✨ add field selector enabled fake client~~ :sparkles add field selector enabled fake client Feb 17, 2020

answer1991 changed the title ~~:sparkles add field selector enabled fake client~~ ✨add field selector enabled fake client Feb 17, 2020

vincepri added this to the v0.5.x milestone Feb 21, 2020

vincepri mentioned this pull request Feb 21, 2020

✨ Add Health Check logic to MachineHealthCheck Reconciler kubernetes-sigs/cluster-api#2250

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 24, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 8, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 6, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 6, 2020

k8s-ci-robot closed this Nov 5, 2020

This was referenced Oct 11, 2022

Add support for fieldSelector to fakeClient #1948

Closed

✨ Add indexes to fake client to mimic field selectors #2025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨add field selector enabled fake client #800

✨add field selector enabled fake client #800

answer1991 commented Feb 15, 2020 •

edited

Loading

k8s-ci-robot commented Feb 15, 2020

k8s-ci-robot commented Feb 15, 2020

k8s-ci-robot commented Feb 15, 2020

answer1991 commented Feb 15, 2020 •

edited

Loading

alvaroaleman left a comment

alvaroaleman Feb 15, 2020

answer1991 Feb 15, 2020

answer1991 Feb 15, 2020

alvaroaleman Feb 15, 2020

answer1991 Feb 15, 2020

answer1991 commented Feb 15, 2020 •

edited

Loading

vincepri commented Feb 15, 2020

alvaroaleman commented Feb 15, 2020

answer1991 commented Feb 17, 2020 •

edited

Loading

vincepri commented Feb 21, 2020

JoelSpeed commented Feb 24, 2020

fejta-bot commented May 24, 2020

vincepri commented Jun 8, 2020

vincepri commented Jun 8, 2020

fejta-bot commented Sep 6, 2020

fejta-bot commented Oct 6, 2020

fejta-bot commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

✨add field selector enabled fake client #800

✨add field selector enabled fake client #800

Conversation

answer1991 commented Feb 15, 2020 • edited Loading

k8s-ci-robot commented Feb 15, 2020

k8s-ci-robot commented Feb 15, 2020

k8s-ci-robot commented Feb 15, 2020

answer1991 commented Feb 15, 2020 • edited Loading

alvaroaleman left a comment

Choose a reason for hiding this comment

alvaroaleman Feb 15, 2020

Choose a reason for hiding this comment

answer1991 Feb 15, 2020

Choose a reason for hiding this comment

answer1991 Feb 15, 2020

Choose a reason for hiding this comment

alvaroaleman Feb 15, 2020

Choose a reason for hiding this comment

answer1991 Feb 15, 2020

Choose a reason for hiding this comment

answer1991 commented Feb 15, 2020 • edited Loading

vincepri commented Feb 15, 2020

alvaroaleman commented Feb 15, 2020

answer1991 commented Feb 17, 2020 • edited Loading

vincepri commented Feb 21, 2020

JoelSpeed commented Feb 24, 2020

fejta-bot commented May 24, 2020

vincepri commented Jun 8, 2020

vincepri commented Jun 8, 2020

fejta-bot commented Sep 6, 2020

fejta-bot commented Oct 6, 2020

fejta-bot commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

answer1991 commented Feb 15, 2020 •

edited

Loading

answer1991 commented Feb 15, 2020 •

edited

Loading

answer1991 commented Feb 15, 2020 •

edited

Loading

answer1991 commented Feb 17, 2020 •

edited

Loading