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 context to EventHandler(s) #2139

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

vincepri
Copy link
Member

This changeset adds a context.Context parameter to every EventHandler call. Most project might use MapFunc specifically to retrieve other objects with clients and potentially enqueue requests to a watching object. A context is useful in these cases to avoid using context.TODO() or Background() which never gets cancelled.

Signed-off-by: Vince Prignano [email protected]

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2023
@vincepri
Copy link
Member Author

/assign @alvaroaleman @sbueringer

Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Nice!

Looking forward to use the ctx in our event handlers :)

Just a few nits

pkg/internal/source/internal_test.go Outdated Show resolved Hide resolved
pkg/internal/source/internal_test.go Outdated Show resolved Hide resolved
pkg/internal/source/eventsource.go Show resolved Hide resolved
This changeset adds a context.Context parameter to every EventHandler
call. Most project might use MapFunc specifically to retrieve other
objects with clients and potentially enqueue requests to a watching
object. A context is useful in these cases to avoid using context.TODO()
or Background() which never gets cancelled.

Signed-off-by: Vince Prignano <[email protected]>
@vincepri vincepri force-pushed the event-handler-context branch from 606f970 to 2464a9d Compare January 24, 2023 15:07
@sbueringer
Copy link
Member

/lgtm

/hold
Not sure if you also want Alvaro to take a look before merging, up to you.

@k8s-ci-robot 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 Jan 24, 2023
@vincepri
Copy link
Member Author

/hold cancel

Let me know if folks have any further feedback, we can continue later

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023
@k8s-ci-robot k8s-ci-robot merged commit 505566d into kubernetes-sigs:master Jan 26, 2023
tiraboschi added a commit to tiraboschi/operator-lib that referenced this pull request May 5, 2023
…alpha.0

- update k8s dependencies to v0.27.1
- update controller-runtime to v0.15.0-alpha.0
- add context to EventHandler calls, see: kubernetes-sigs/controller-runtime#2139

Signed-off-by: Simone Tiraboschi <[email protected]>
tiraboschi added a commit to tiraboschi/operator-lib that referenced this pull request May 22, 2023
…beta.0

- update k8s dependencies to v0.27.2
- update controller-runtime to v0.15.0-beta.0
- add context to EventHandler calls, see: kubernetes-sigs/controller-runtime#2139

Signed-off-by: Simone Tiraboschi <[email protected]>
tiraboschi added a commit to tiraboschi/operator-lib that referenced this pull request May 22, 2023
…beta.0

- update k8s dependencies to v0.27.2
- update controller-runtime to v0.15.0-beta.0
- add context to EventHandler calls, see: kubernetes-sigs/controller-runtime#2139

Signed-off-by: Simone Tiraboschi <[email protected]>
tiraboschi added a commit to tiraboschi/operator-lib that referenced this pull request May 23, 2023
- update k8s dependencies to v0.27.2
- update controller-runtime to v0.15.0
- add context to EventHandler calls, see: kubernetes-sigs/controller-runtime#2139

Signed-off-by: Simone Tiraboschi <[email protected]>
openshift-merge-robot pushed a commit to operator-framework/operator-lib that referenced this pull request Jun 7, 2023
…114)

- update k8s dependencies to v0.27.2
- update controller-runtime to v0.15.0
- add context to EventHandler calls, see: kubernetes-sigs/controller-runtime#2139

Signed-off-by: Simone Tiraboschi <[email protected]>
lyarwood added a commit to lyarwood/ssp-operator that referenced this pull request Jun 26, 2023
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0

* Make `*http.Client` configurable and use/share the same client by
  default kubernetes-sigs/controller-runtime#2122

* Remove dependency injection functions
  kubernetes-sigs/controller-runtime#2134,
  kubernetes-sigs/controller-runtime#2120

* Add context to EventHandler(s)
  kubernetes-sigs/controller-runtime#2139

* `Validator` and `CustomValidator` interfaces have been modified to
  allow returning warnings
  kubernetes-sigs/controller-runtime#2014

* operator-framework is also pinned to ecb9be48837 until a new release
  is cut supporting controller-runtime v0.15.0

Signed-off-by: Lee Yarwood <[email protected]>
ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023
lyarwood added a commit to lyarwood/ssp-operator that referenced this pull request Jun 28, 2023
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0

* Make `*http.Client` configurable and use/share the same client by
  default kubernetes-sigs/controller-runtime#2122

* Remove dependency injection functions
  kubernetes-sigs/controller-runtime#2134,
  kubernetes-sigs/controller-runtime#2120

* Add context to EventHandler(s)
  kubernetes-sigs/controller-runtime#2139

* `Validator` and `CustomValidator` interfaces have been modified to
  allow returning warnings
  kubernetes-sigs/controller-runtime#2014

* operator-framework is also pinned to ecb9be48837 until a new release
  is cut supporting controller-runtime v0.15.0

Signed-off-by: Lee Yarwood <[email protected]>
lyarwood added a commit to lyarwood/ssp-operator that referenced this pull request Aug 9, 2023
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0

* Make `*http.Client` configurable and use/share the same client by
  default kubernetes-sigs/controller-runtime#2122

* Remove dependency injection functions
  kubernetes-sigs/controller-runtime#2134,
  kubernetes-sigs/controller-runtime#2120

* Add context to EventHandler(s)
  kubernetes-sigs/controller-runtime#2139

* `Validator` and `CustomValidator` interfaces have been modified to
  allow returning warnings
  kubernetes-sigs/controller-runtime#2014

* operator-framework is also pinned to ecb9be48837 until a new release
  is cut supporting controller-runtime v0.15.0

Signed-off-by: Lee Yarwood <[email protected]>
rriski added a commit to aiven/aiven-operator that referenced this pull request Jan 24, 2024
- Removal of deprecated manager options:
  - kubernetes-sigs/controller-runtime#2422
- Context added to `EnqueueRequestsFromMapFunc`
  - kubernetes-sigs/controller-runtime#2139
rriski added a commit to aiven/aiven-operator that referenced this pull request Jan 24, 2024
- Removal of deprecated manager options:
  - kubernetes-sigs/controller-runtime#2422
- Context added to `EnqueueRequestsFromMapFunc`
  - kubernetes-sigs/controller-runtime#2139
rriski added a commit to aiven/aiven-operator that referenced this pull request Jan 24, 2024
- Removal of deprecated manager options:
  - kubernetes-sigs/controller-runtime#2422
- Context added to `EnqueueRequestsFromMapFunc`
  - kubernetes-sigs/controller-runtime#2139
rriski added a commit to aiven/aiven-operator that referenced this pull request Jan 24, 2024
- Removal of deprecated manager options:
  - kubernetes-sigs/controller-runtime#2422
- Context added to `EnqueueRequestsFromMapFunc`
  - kubernetes-sigs/controller-runtime#2139
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 "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants