-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
🐛 Check to see if custom source implements fmt.Stringer when logging #3068
🐛 Check to see if custom source implements fmt.Stringer when logging #3068
Conversation
Signed-off-by: Troy Connor <[email protected]>
8ef597e
to
707562c
Compare
/test pull-controller-runtime-test |
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.
Great. Looks very elegant
@dongjiang1989: changing LGTM is restricted to collaborators In 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. |
@@ -175,7 +175,7 @@ func (c *Controller[request]) Start(ctx context.Context) error { | |||
// caches. | |||
errGroup := &errgroup.Group{} | |||
for _, watch := range c.startWatches { | |||
log := c.LogConstructor(nil).WithValues("source", fmt.Sprintf("%s", watch)) | |||
log := c.LogConstructor(nil).WithValues("source", watch.String()) |
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.
It is an interesting issue - How about rather then requiring the implementation of fmt.Stringer
, we check if it is implemented and if yes use that, otherwise use the Type (i.E. fmt.Sprintf("%T", watch)
)? That way we avoid the breaking change
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.
thanks!
LGTM label has been added. Git tree hash: a414c38213cf8ff7a9d5fe0e187d2c9bfbc50d2d
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, dongjiang1989, troy0820 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 |
/cherry-pick release-0.19 |
@troy0820: #3068 failed to apply on top of branch "release-0.19":
In 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. |
/cherry-pick release-0.20 |
@troy0820: new pull request created: #3077 In 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. |
When using a custom source, the error when starting will fail because the
fmt.Stringer
interface is not implemented by the source. The builtin sources have already implanted this interface allow the below line to not fail:Resolves #3057