-
Notifications
You must be signed in to change notification settings - Fork 25
Use new client-go functions for contextual logging #663
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
Conversation
Signed-off-by: Tim Ramlot <[email protected]>
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.
Pull Request Overview
This PR replaces stop-channel parameters with context.Context
across data gatherer implementations to enable contextual logging and aligns informer usage with client-go’s context-based APIs.
- Refactors
Run
andWaitForCacheSync
signatures in local, discovery, dynamic, and dummy gatherers to acceptcontext.Context
. - Updates agent code and tests to pass contexts instead of channels.
- Switches informer handler registration to
AddEventHandlerWithOptions
with a logger and usesRunWithContext
.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pkg/datagatherer/local/local.go | Changed Run /WaitForCacheSync to accept context.Context . |
pkg/datagatherer/k8s/dynamic.go | Removed stored ctx field; added RunWithContext , HandlerOptions logger; updated WaitForCacheSync . |
pkg/datagatherer/k8s/dynamic_test.go | Adapted tests to use new signatures and removed ctx assertions. |
pkg/datagatherer/k8s/discovery.go | Changed Run /WaitForCacheSync to accept context.Context . |
pkg/datagatherer/datagatherer.go | Updated DataGatherer interface to use context.Context . |
pkg/agent/run.go | Updated calls to Run /WaitForCacheSync to pass contexts. |
pkg/agent/dummy_data_gatherer.go | Changed Run /WaitForCacheSync to accept context.Context . |
Comments suppressed due to low confidence (2)
pkg/datagatherer/k8s/dynamic.go:271
- The comment here still refers to
stopCh
, butRun
now takes acontext.Context
. Please update the doc to reflect that it blocks until the context is done.
// until the stopCh is closed.
pkg/agent/run.go:191
- This comment refers to a channel; since
DataGatherer.Run
now usescontext.Context
, consider updating the wording to describe blocking onctx.Done()
.
// blocks until the supplied channel is closed.
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.
Looks good. I prefer the use of context arguments instead of done channels...it's more familiar to me these days.
But I'd like to see some example output, demonstrating the new logging that is generated by this change and what level it is logged at.
This change has no effects on the logs when there are not critical errors (crashes in OnAdd etc). Also, since we were already setting the global logger, json logging was already working for the messages that were logged by the informer. |
Use
Context
s instead ofstopCh
when working with informers, such that client-go can use contextual logging.See https://pkg.go.dev/k8s.io/[email protected]/tools/cache#SharedInformer for more info.