Fix mDNS package logging #849
Conversation
openshift-ci
bot
added
the
approved
|
changes look good to me. @mangelajo nit: since this is about log change, do you think it is worth adding some debug level log that can be enabled for development testing? one example is I had to add debug all the way down to the server.go |
|
I'm good with that but let's do it in a separate PR. This is about fixing the bug which has been around for a shameful amount of time I don't want to recognize at this time %). |
|
@ggiguash @zshi-redhat does it look ok to lgtm ? I will be PTO after today. |
pkg/mdns/controller.go
Outdated
There was a problem hiding this comment.
Should we capitalize the first word (Starting) to make is consistent with other messages?
pkg/mdns/controller.go
Outdated
There was a problem hiding this comment.
ips -> IP addresses
or
IPs
pkg/mdns/routes.go
Outdated
There was a problem hiding this comment.
It's an error, so should we say something like the following?
Failed waiting for...
pkg/mdns/routes.go
Outdated
There was a problem hiding this comment.
semicolon missing? "found: %s"
pkg/mdns/routes.go
Outdated
There was a problem hiding this comment.
We may want to be consistent about mDNS messages prefix: "mDNS: ..." or "mDNS ..."
|
Good feedback @ggiguash let me handle it :) |
The logging parameters were set for structured logging (InfoS) in some cases, while Infof is being used (same for Errorf). Closes: https://issues.redhat.com/browse/USHIFT-263
|
@ggiguash done :) |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, mangelajo 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 |
|
/retest |
|
@mangelajo: all tests passed! Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
5 participants
The logging parameters were set for structured logging (InfoS) in
some cases, while Infof is being used (same for Errorf).
Closes: https://issues.redhat.com/browse/USHIFT-263
Which issue(s) this PR addresses:
Closes #