⚠️ Add AnnotatedEventRecorderProvider interface - #3509
kubernetes-prow[bot] merged 2 commits into
Conversation
|
Welcome @adri1197! |
|
Hi @adri1197. 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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
2e5cff1 to
56f611b
Compare
|
@matheuscscp: changing LGTM is restricted to collaborators DetailsIn response to this:
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adri1197, matheuscscp The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@adri1197 We just bumped to v1.37 alpha, so I think this PR can be rebased and moved out of draft |
|
/ok-to-test |
56f611b to
4a38bfc
Compare
@sbueringer since you mentioned this can be rebased and moved out of draft now that we're on v1.37 alpha — would it be acceptable to add the method directly to the Provider interface as a breaking change for the next minor (given controller-runtime's v0.x semver policy allows breaking changes between minors)? Or should we stick with the separate? interface approach from the PR description to keep it non-breaking? |
| return c.recorderProvider.GetEventRecorder(name) | ||
| } | ||
|
|
||
| func (c *cluster) GetAnnotatedEventRecorder(name string) events.AnnotatedEventRecorder { |
There was a problem hiding this comment.
I'm wondering if we should really have an additional Get*EventRecorder method instead of just combining both "new" event recorder in an interface in controller-runtime.
I think this is an acceptable breaking change either way
I assume it's easier to use if we just have one method / one recorder. Because folks then don't have to get and pass around two different recorders if they want to use both methods.
@alvaroaleman Do you have an opinion on this?
Context: The old upstream EventRecorder had Event / Eventf / AnnotatedEventf methods. The new API has one interface each for Eventf / AnnotatedEventf
There was a problem hiding this comment.
Friendly ping @sbueringer @alvaroaleman 👋 — the rework combining both into a single recorder.EventRecorder interface (per @sbueringer's suggestion) has been pushed.
Would you have a chance to take another look?
Thanks! 😃
There was a problem hiding this comment.
Thx! I'll try to take a look soon, probably tomorrow
Sorry just saw this message now. This is the same point I'm bringing up #3509 (comment), right? EDIT: Ah I think it's a slightly different point |
No really, it's exactly the answer that I wanted to get 😃. I had some doubts about having different Recorders (well, Providers) per interface. I've reworked this to combine both into a single |
Add the missing AnnotatedEventf as a new interface AnnotatedEventRecorder under cluster, manager, and recorder provider Signed-off-by: Adrian Fernandez De La Torre <adri1197@gmail.com>
…face Replace separate GetEventRecorder and GetAnnotatedEventRecorder methods with a single GetEventRecorder that returns a combined recorder.EventRecorder interface embedding both events.EventRecorder and events.AnnotatedEventRecorder. This lets callers use one recorder for both Eventf and AnnotatedEventf without needing to obtain and pass around two separate recorders. Signed-off-by: Adrian Fernandez De La Torre <adri1197@gmail.com>
a4c69c4 to
588f17a
Compare
|
@adri1197: 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. DetailsInstructions 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. |
|
Thank you very much!! /lgtm /assign @alvaroaleman |
|
LGTM label has been added. DetailsGit tree hash: 1d5558c23aedb563ba9492e9b704027b644c5d8f |
|
Marked the change as breaking since it changes the interface, but otherwise lgtm, thank you |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adri1197, alvaroaleman, matheuscscp 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 |
b2719fd
into
kubernetes-sigs:main
Adds support for creating event recorders that can attach custom annotations to events,
mirroring the
AnnotatedEventfcapability recently added toclient-go/tools/events(see kubernetes/kubernetes#138103).
This introduces a new
AnnotatedEventRecorderProviderinterface inpkg/recorderratherthan adding a method to the existing
Providerinterface, so this is a backwards-compatiblechange. Consumers can type-assert the manager/cluster to access the new functionality:
Changes: