-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Log controller name in predicates #10959
Conversation
@sbueringer regarding the kind value being empty I see multiple issues regarding Typemeta being empty kubernetes-sigs/controller-runtime#1735 and needs an upstream fix. Meanwhile I think we can fetch the GVK with help of client like client.GroupVersionKindFor(obj), But which might require predicate signature change or creating a new predicate. Should I do that? Could you please help me If any other better way of fetching find GVK or making object contain GVK. |
@Karthik-K-N I'll look into, but I'll need some time |
c27fa8b
to
cd23215
Compare
@sbueringer I thought of using |
I would use "controller". This should intentionally match up with the controller key/value pair of other code of the same controller (which controller-runtime does for us for everything that gets a logger out of the context passed into |
cd23215
to
2c3bb21
Compare
I got your point and made the changes to use controller as the key, Earlier my intention was to uniquely identify predicate logs using that key. |
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.
Thx, two nits
2c3bb21
to
02410f2
Compare
Thank you! /lgtm |
LGTM label has been added. Git tree hash: 893b1af962ca8d1e89508efdb21e42a3c2ae9b58
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
/retest |
Changed the PR description to re-trigger the title verifier. We merged a change there which had a bug, but that was fixed just now |
What this PR does / why we need it:
This PR tries to improve the logging of predicate to include more key value pairs.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #10818
/area logging