⚠ update Informers interface to accept context #830

djzager · 2020-02-27T02:49:11Z

Update all functions on the Informers interface (and IndexField on
FieldIndexer interface) to accept a context.

Fixes #828

k8s-ci-robot · 2020-02-27T02:49:19Z

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

djzager · 2020-02-27T02:51:34Z

@DirectXMan12 follow-up to #663

djzager · 2020-02-27T02:52:34Z

Made this ⚠️ since it changes the function signatures on public interfaces.

alvaroaleman

/ok-to-test

alvaroaleman · 2020-02-27T10:08:11Z

pkg/cache/cache.go

@@ -51,11 +52,11 @@ type Cache interface {
 type Informers interface {
 	// GetInformer fetches or constructs an informer for the given object that corresponds to a single
 	// API kind and resource.
-	GetInformer(obj runtime.Object) (Informer, error)
+	GetInformer(ctx context.Context, obj runtime.Object) (Informer, error)


@DirectXMan12 what is our policy for breaking changes? IMHO this change would be extremely great, if we want to give a longer deprecation period we could probably add a new InformersV2 interface and start using that internally?

We'll probably need to add this to v0.6 if it's a breaking change

djzager · 2020-02-27T14:04:35Z

@alvaroaleman I made it WIP wanting to add some form of test (outside of the tests that were update so it would build) but nothing stands out as a good candidate. Any ideas?

alvaroaleman · 2020-02-27T14:13:12Z

@alvaroaleman I made it WIP wanting to add some form of test (outside of the tests that were update so it would build) but nothing stands out as a good candidate. Any ideas?

A testcase that tries to get an informer from an envtest cluster with a context with super low timeout (1 ms or sth), then verifying the error is context canceled?

Update all functions on the Informers interface (and IndexField on FieldIndexer interface) to accept a context.

joelanford · 2020-03-19T01:47:56Z

pkg/source/source.go

@@ -98,7 +99,7 @@ func (ks *Kind) Start(handler handler.EventHandler, queue workqueue.RateLimiting
 	}

 	// Lookup the Informer from the Cache and add an EventHandler which populates the Queue
-	i, err := ks.cache.GetInformer(ks.Type)
+	i, err := ks.cache.GetInformer(context.TODO(), ks.Type)


Do we need to add context.Context to the Source interface's Start method to make this cancellable?

I guess, but maybe we can do that in a follow-up?

vincepri

/approve
/assign @alvaroaleman @DirectXMan12

k8s-ci-robot · 2020-03-26T17:14:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: djzager, vincepri

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

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

alvaroaleman

/lgtm
/hold
@vincepri are we good with merging breaking changes?

alvaroaleman · 2020-03-27T14:16:33Z

pkg/source/source.go

@@ -98,7 +99,7 @@ func (ks *Kind) Start(handler handler.EventHandler, queue workqueue.RateLimiting
 	}

 	// Lookup the Informer from the Cache and add an EventHandler which populates the Queue
-	i, err := ks.cache.GetInformer(ks.Type)
+	i, err := ks.cache.GetInformer(context.TODO(), ks.Type)


I guess, but maybe we can do that in a follow-up?

vincepri · 2020-03-27T14:49:50Z

yep, we discussed at yesterday's meeting and we opened master to v0.6

alvaroaleman · 2020-03-27T15:15:34Z

/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. 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 27, 2020

k8s-ci-robot requested review from mengqiy and vincepri February 27, 2020 02:49

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 27, 2020

alvaroaleman reviewed Feb 27, 2020

View reviewed changes

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

⚠ update Informers interface to accept context

b6d18c7

Update all functions on the Informers interface (and IndexField on FieldIndexer interface) to accept a context.

djzager force-pushed the ctx-informers branch from c18717b to b6d18c7 Compare February 27, 2020 18:50

djzager changed the title ~~WIP: ⚠ update Informers interface to accept context~~ ⚠ update Informers interface to accept context Feb 27, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2020

vincepri added this to the v0.6.0 milestone Feb 27, 2020

joelanford reviewed Mar 19, 2020

View reviewed changes

vincepri approved these changes Mar 26, 2020

View reviewed changes

k8s-ci-robot assigned alvaroaleman and DirectXMan12 Mar 26, 2020

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

alvaroaleman approved these changes Mar 27, 2020

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 27, 2020

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

k8s-ci-robot merged commit e0b187f into kubernetes-sigs:master Mar 27, 2020

djzager mentioned this pull request Mar 27, 2020

Add context.Context to Source interface's Start method to make cancellable #879

Closed

⚠ update Informers interface to accept context #830

⚠ update Informers interface to accept context #830

Uh oh!

Conversation

djzager commented Feb 27, 2020

Uh oh!

k8s-ci-robot commented Feb 27, 2020

Uh oh!

djzager commented Feb 27, 2020

Uh oh!

djzager commented Feb 27, 2020

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

vincepri Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

djzager commented Feb 27, 2020

Uh oh!

alvaroaleman commented Feb 27, 2020

Uh oh!

joelanford Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 26, 2020

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

vincepri commented Mar 27, 2020

Uh oh!

alvaroaleman commented Mar 27, 2020

Uh oh!

Uh oh!