-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 TypedReconciler #2799
⚠️ Add TypedReconciler #2799
Conversation
/hold |
92ddc15
to
e8833b6
Compare
@@ -64,70 +65,70 @@ type EventHandler TypedEventHandler[client.Object] | |||
// Most users shouldn't need to implement their own TypedEventHandler. | |||
// | |||
// TypedEventHandler is experimental and subject to future change. | |||
type TypedEventHandler[T any] interface { | |||
type TypedEventHandler[object any, request comparable] interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main annoyance I found with this change for standard k8s controllers is this, when using a typed eventhandler to have the added typesafety in there it is now also needed to pass the request type. Removing it would mean it is impossible to use a custom request type with source.Kind
though, which is for example useful when having watches in different clusters
I've tried this change with some internal projects and the changes required there were minimal and mostly around the interface change in |
2a4db5d
to
40c6b5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really really nice work! :)
Two high-level points:
- would be good to have consistent type parameter & corresponding argument names
- might be good to take another look if we have enough test coverage for the new "Typed" types
2b0b941
to
0f3a3cb
Compare
@@ -0,0 +1,60 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions this can be a good use for multiple clusters. Could we add an example that expands on the current reconcile.Request
to add Cluster information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added one under examples/multiclustersync
This change adds a TypedReconciler which allows to customize the type being used in the workqueue. There is a number of situations where a custom type might be better than the default `reconcile.Request`: * Multi-Cluster controllers might want to put the clusters in there * Some controllers do not reconcile individual resources of a given type but all of them at once, for example IngressControllers might do this * Some controllers do not operate on Kubernetes resources at all
@alvaroaleman: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
/hold cancel |
LGTM label has been added. Git tree hash: e68ce7d6eaa2729f0fc45cc1de0e7a62ef7512d7
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, 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 |
Nice additional example, thx for adding it! |
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616
Related upstream change: - kubernetes-sigs/controller-runtime#2799
- Changes to reconciler-related types pursuant to kubernetes-sigs/controller-runtime#2799 - Removal of yellow squiggles HIVE-2616 (cherry picked from commit 08e4987)
…pdates (#943) * Update to support the TypedEventHandler interface Related upstream change: - kubernetes-sigs/controller-runtime#2799 * Support the sha512sum Sprig template function Bumps the gomod-backward-compatible group with 11 updates in the / directory: | Package | From | To | | --- | --- | --- | | [cloud.google.com/go/compute/metadata](https://github.com/googleapis/google-cloud-go) | `0.5.0` | `0.5.2` | | [github.com/Masterminds/sprig/v3](https://github.com/Masterminds/sprig) | `3.2.3` | `3.3.0` | | [github.com/gruntwork-io/terratest](https://github.com/gruntwork-io/terratest) | `0.47.0` | `0.47.2` | | [github.com/hashicorp/hcp-sdk-go](https://github.com/hashicorp/hcp-sdk-go) | `0.110.0` | `0.115.0` | | [github.com/onsi/gomega](https://github.com/onsi/gomega) | `1.34.1` | `1.34.2` | | [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) | `1.20.3` | `1.20.4` | | [golang.org/x/crypto](https://github.com/golang/crypto) | `0.26.0` | `0.27.0` | | [google.golang.org/api](https://github.com/googleapis/google-api-go-client) | `0.192.0` | `0.199.0` | | [k8s.io/apiextensions-apiserver](https://github.com/kubernetes/apiextensions-apiserver) | `0.30.3` | `0.31.1` | | [sigs.k8s.io/controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) | `0.18.4` | `0.19.0` | | [nhooyr.io/websocket](https://github.com/nhooyr/websocket-old) | `1.8.11` | `1.8.17` | Updates `cloud.google.com/go/compute/metadata` from 0.5.0 to 0.5.2 - [Release notes](https://github.com/googleapis/google-cloud-go/releases) - [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md) - [Commits](googleapis/google-cloud-go@v0.5.0...auth/v0.5.2) Updates `github.com/Masterminds/sprig/v3` from 3.2.3 to 3.3.0 - [Release notes](https://github.com/Masterminds/sprig/releases) - [Changelog](https://github.com/Masterminds/sprig/blob/master/CHANGELOG.md) - [Commits](Masterminds/sprig@v3.2.3...v3.3.0) Updates `github.com/gruntwork-io/terratest` from 0.47.0 to 0.47.2 - [Release notes](https://github.com/gruntwork-io/terratest/releases) - [Commits](gruntwork-io/terratest@v0.47.0...v0.47.2) Updates `github.com/hashicorp/hcp-sdk-go` from 0.110.0 to 0.115.0 - [Release notes](https://github.com/hashicorp/hcp-sdk-go/releases) - [Changelog](https://github.com/hashicorp/hcp-sdk-go/blob/main/CHANGELOG.md) - [Commits](hashicorp/hcp-sdk-go@v0.110.0...v0.115.0) Updates `github.com/onsi/gomega` from 1.34.1 to 1.34.2 - [Release notes](https://github.com/onsi/gomega/releases) - [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md) - [Commits](onsi/gomega@v1.34.1...v1.34.2) Updates `github.com/prometheus/client_golang` from 1.20.3 to 1.20.4 - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.20.3...v1.20.4) Updates `golang.org/x/crypto` from 0.26.0 to 0.27.0 - [Commits](golang/crypto@v0.26.0...v0.27.0) Updates `google.golang.org/api` from 0.192.0 to 0.199.0 - [Release notes](https://github.com/googleapis/google-api-go-client/releases) - [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md) - [Commits](googleapis/google-api-go-client@v0.192.0...v0.199.0) Updates `k8s.io/apiextensions-apiserver` from 0.30.3 to 0.31.1 - [Release notes](https://github.com/kubernetes/apiextensions-apiserver/releases) - [Commits](kubernetes/apiextensions-apiserver@v0.30.3...v0.31.1) Updates `k8s.io/apimachinery` from 0.30.3 to 0.31.1 - [Commits](kubernetes/apimachinery@v0.30.3...v0.31.1) Updates `k8s.io/client-go` from 0.30.3 to 0.31.1 - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.30.3...v0.31.1) Updates `k8s.io/utils` from 0.0.0-20230726121419-3b25d923346b to 0.0.0-20240711033017-18e509b52bc8 - [Commits](https://github.com/kubernetes/utils/commits) Updates `sigs.k8s.io/controller-runtime` from 0.18.4 to 0.19.0 - [Release notes](https://github.com/kubernetes-sigs/controller-runtime/releases) - [Changelog](https://github.com/kubernetes-sigs/controller-runtime/blob/main/RELEASE.md) - [Commits](kubernetes-sigs/controller-runtime@v0.18.4...v0.19.0) Updates `nhooyr.io/websocket` from 1.8.11 to 1.8.17 - [Release notes](https://github.com/nhooyr/websocket-old/releases) - [Commits](https://github.com/nhooyr/websocket-old/commits/v1.8.17) --- updated-dependencies: - dependency-name: cloud.google.com/go/compute/metadata dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gomod-backward-compatible - dependency-name: github.com/Masterminds/sprig/v3 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: github.com/gruntwork-io/terratest dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gomod-backward-compatible - dependency-name: github.com/hashicorp/hcp-sdk-go dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: github.com/onsi/gomega dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gomod-backward-compatible - dependency-name: github.com/prometheus/client_golang dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gomod-backward-compatible - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: google.golang.org/api dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: k8s.io/apiextensions-apiserver dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: k8s.io/apimachinery dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: k8s.io/client-go dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: k8s.io/utils dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gomod-backward-compatible - dependency-name: sigs.k8s.io/controller-runtime dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gomod-backward-compatible - dependency-name: nhooyr.io/websocket dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gomod-backward-compatible ... Signed-off-by: dependabot[bot] <support@github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ben Ash <bash@hashicorp.com>
This change adds a TypedReconciler which allows to customize the type being used in the workqueue. There is a number of situations where a custom type might be better than the default
reconcile.Request
: