You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
controller-runtime v0.23 deprecated manager.GetEventRecorderFor and the legacy k8s.io/client-go/tools/record.EventRecorder API in favor of the newer k8s.io/client-go/tools/events.EventRecorder. The deprecation notice says the old API will be removed in a future release.
operatorpkg's status.NewController and status.NewGenericObjectController currently take record.EventRecorder, which means downstream consumers can't drop their own usage of GetEventRecorderFor — they're forced to suppress the SA1019 lint at every call site.
For example, kubernetes-sigs/karpenter#2951 currently carries two //nolint:staticcheck directives just to keep using operatorpkg after bumping controller-runtime.
Proposal
Migrate operatorpkg's status controller to the new events.EventRecorder API. Two possible approaches:
Hard swap. Change the status.NewController / status.NewGenericObjectController signatures to take events.EventRecorder. Breaking change for callers, but compile-time, with a one-line migration (mgr.GetEventRecorderFor(name) → mgr.GetEventRecorder(name)).
Parallel constructor. Add NewControllerWithEventsRecorder (or similar) alongside the existing one, and deprecate the old constructor. Slower migration, no breaking change. Matches the pattern from split service account from ExpectApplied #190 where a downstream-impacting change was reworked into an additive split.
Implementation surface (audited)
The change is small:
status/controller.go: import swap, eventRecorder field type, two constructor signatures, three internal eventRecorder.Event(...) → Eventf(...) translations. Reusable action strings: Finalize and TransitionCondition.
status/controller_test.go: swap record.FakeRecorder for events.FakeRecorder. The new fake produces the same <type> <reason> <note> channel format, so existing assertion strings stay valid unchanged.
No other call sites in operatorpkg use record.EventRecorder.
~30 repos depend on operatorpkg, but the real blast radius for a status.NewController signature change is roughly:
~10 karpenter cloud-provider repos that bump operatorpkg via karpenter's go.sum and would migrate in lockstep
~5 standalone consumers (kaito, eks-node-monitoring-agent, tensor-fusion, gpu-provisioner, grit) that may or may not actually call status.NewController directly
A compile-time signature break is loud and easy to fix at every call site, which makes the hard swap defensible. Happy to defer to maintainer preference on which approach to take.
Background
controller-runtimev0.23 deprecatedmanager.GetEventRecorderForand the legacyk8s.io/client-go/tools/record.EventRecorderAPI in favor of the newerk8s.io/client-go/tools/events.EventRecorder. The deprecation notice says the old API will be removed in a future release.operatorpkg's
status.NewControllerandstatus.NewGenericObjectControllercurrently takerecord.EventRecorder, which means downstream consumers can't drop their own usage ofGetEventRecorderFor— they're forced to suppress theSA1019lint at every call site.For example, kubernetes-sigs/karpenter#2951 currently carries two
//nolint:staticcheckdirectives just to keep using operatorpkg after bumping controller-runtime.Proposal
Migrate operatorpkg's status controller to the new
events.EventRecorderAPI. Two possible approaches:status.NewController/status.NewGenericObjectControllersignatures to takeevents.EventRecorder. Breaking change for callers, but compile-time, with a one-line migration (mgr.GetEventRecorderFor(name)→mgr.GetEventRecorder(name)).NewControllerWithEventsRecorder(or similar) alongside the existing one, and deprecate the old constructor. Slower migration, no breaking change. Matches the pattern from split service account from ExpectApplied #190 where a downstream-impacting change was reworked into an additive split.Implementation surface (audited)
The change is small:
status/controller.go: import swap,eventRecorderfield type, two constructor signatures, three internaleventRecorder.Event(...)→Eventf(...)translations. Reusable action strings:FinalizeandTransitionCondition.status/controller_test.go: swaprecord.FakeRecorderforevents.FakeRecorder. The new fake produces the same<type> <reason> <note>channel format, so existing assertion strings stay valid unchanged.record.EventRecorder.A working draft of the hard-swap approach is on https://github.com/jamesmt-aws/operatorpkg/tree/chore/migrate-events-recorder (was opened as #205, closed in favor of this tracking issue so the design question isn't blocked on a specific PR shape).
Downstream blast radius
~30 repos depend on operatorpkg, but the real blast radius for a
status.NewControllersignature change is roughly:go.sumand would migrate in lockstepstatus.NewControllerdirectlyA compile-time signature break is loud and easy to fix at every call site, which makes the hard swap defensible. Happy to defer to maintainer preference on which approach to take.
Downstream cleanup
Once this lands, the karpenter follow-up at https://github.com/jamesmt-aws/karpenter/tree/chore/migrate-event-recorder-api can be opened to drop the two nolints.