Use constructors to create event handlers#8314
Use constructors to create event handlers#8314cert-manager-prow[bot] merged 2 commits intocert-manager:masterfrom
Conversation
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
cdc1c7f to
a677cd5
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the event handler creation pattern in cert-manager controllers from using exported struct types with struct literals to using unexported types with constructor functions. The changes improve API encapsulation by hiding implementation details and providing a cleaner interface for creating QueuingEventHandler and BlockingEventHandler instances.
Key changes:
- Convert
QueuingEventHandlerandBlockingEventHandlerfrom exported structs to unexported structs with exported constructor functions - Update all controller registrations (20+ files) to use the new constructor functions instead of struct literals
- Minor dependency updates (ginkgo v2.22.0 and gomega v1.36.1)
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/util.go | Refactored event handler types to unexported structs with exported constructor functions; changed method receivers from pointer to value; made struct fields unexported |
| pkg/controller/issuers/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/clusterissuers/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificatesigningrequests/selfsigned/selfsigned.go | Updated to use BlockingEventHandler() constructor function |
| pkg/controller/certificatesigningrequests/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificatesigningrequests/acme/acme.go | Updated to use BlockingEventHandler() constructor function |
| pkg/controller/certificates/trigger/trigger_controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificates/revisionmanager/revisionmanager_controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificates/requestmanager/requestmanager_controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificates/readiness/readiness_controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificates/metrics/certificates.go | Updated to use QueuingEventHandler() constructor function |
| pkg/controller/certificates/keymanager/keymanager_controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificates/issuing/issuing_controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificaterequests/selfsigned/selfsigned.go | Updated to use BlockingEventHandler() constructor function |
| pkg/controller/certificaterequests/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificaterequests/approver/approver.go | Updated to use QueuingEventHandler() constructor function |
| pkg/controller/certificaterequests/acme/acme.go | Updated to use BlockingEventHandler() constructor function |
| pkg/controller/certificate-shim/ingresses/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/certificate-shim/gateways/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/acmeorders/controller.go | Updated to use QueuingEventHandler() and BlockingEventHandler() constructor functions |
| pkg/controller/acmechallenges/controller.go | Updated to use QueuingEventHandler() constructor function |
| cmd/controller/go.mod | Added indirect dependencies for ginkgo v2.22.0 and gomega v1.36.1 |
| cmd/controller/go.sum | Updated checksums for ginkgo and gomega dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| // QueuingEventHandler is an implementation of cache.ResourceEventHandler that |
There was a problem hiding this comment.
This is under pkg, which means it might be used in use by anyone on the internet.
I know that we tell people that we don't guaranty any API stability for most stuff under pkg (importing-cert-manager-as-a-module) but I'd prefer knowing before breaking folks; if we do break people, I'd prefer we let them know upfront.
I found two projets relying on this particular piece of the API:
- nginx-openid-connect/nginx-oidc-kubernetes (cm_controller.go#L128)
- nginx/kubernetes-ingress (cm_controller.go#L125)
Seems like both projects are owned by F5. I'd be OK with this PR if we let them know right away e.g. us opening an issue on both projects and letting them know that cert-manager 1.20 will have this breaking change.
maelvls
left a comment
There was a problem hiding this comment.
/lgtm
I'm good as long as we give prior notice to the F5 folks who maintain the two projects that we will be breaking in v1.20.0 with this PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maelvls The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8318aff
into
cert-manager:master
refactor required for #8261
Kind
/kind cleanup
Release Note
CyberArk tracker: VC-47743