fix: SetLogger via klog.SetLogger will output an unexpected newline #378

aimuz · 2023-05-11T14:37:43Z

klog always adds a newline to the msg. klog will work fine without klog.

Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added.

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1])

fixes #370

Signed-off-by: aimuz [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

NONE

k8s-ci-robot · 2023-05-11T14:37:51Z

This issue is currently awaiting triage.

If klog contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

dims · 2023-05-11T15:42:24Z

/assign @pohly

pohly · 2023-05-15T08:37:39Z

@aimuz: to reproduce the test failures, use go test ./... && (cd examples && go test ./...).

aimuz · 2023-05-15T10:57:42Z

thanks you @pohly

aimuz · 2023-05-17T01:58:30Z

PR is ready, please feel free to review

pohly · 2023-05-17T05:58:34Z

klog.go

@@ -873,6 +873,9 @@ func (l *loggingT) output(s severity.Severity, logger *logWriter, buf *buffer.Bu
 		if logger.writeKlogBuffer != nil {
 			logger.writeKlogBuffer(data)
 		} else {
+			if data[len(data)-1] == '\n' {


I know, data can probably never be empty, but perhaps check for it anyway?

Yes, it is necessary that

done

pohly

Looks good. Can you squash into one commit with a useful commit message?

pohly · 2023-05-17T08:34:36Z

The first commit message is already okay, no need to edit it.

klog always adds a newline to the msg. klog will work fine without klog. Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added. step1: klog.Info("hello world") step2: msg = msg + "\n" step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1]) fix kubernetes#370 Signed-off-by: aimuz <[email protected]>

aimuz · 2023-05-17T10:06:54Z

done

pohly

/lgtm
/approve

Dropping a newline at the end of the message when using klog calls is an intentional improvement (kubernetes/klog#378) Kubernetes-commit: 878d037d3ba8fc4f11bf45a6cf5a66301ba89d82

Fix for kubernetes/klog#378. Signed-off-by: Steve Kriss <[email protected]>

Dropping a newline at the end of the message when using klog calls is an intentional improvement (kubernetes/klog#378)

Dropping a newline at the end of the message when using klog calls is an intentional improvement (kubernetes/klog#378)

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 11, 2023

k8s-ci-robot requested review from harshanarayana and pohly May 11, 2023 14:37

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label May 11, 2023

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 11, 2023

aimuz force-pushed the fix-370 branch from 8aba960 to 37def3b Compare May 11, 2023 14:38

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 11, 2023

aimuz force-pushed the fix-370 branch 2 times, most recently from b56182d to ef55402 Compare May 11, 2023 14:44

k8s-ci-robot assigned pohly May 11, 2023

aimuz changed the title ~~fix: SetLogger via klog.SetLogger will output an unexpected newline~~ wip: fix: SetLogger via klog.SetLogger will output an unexpected newline May 12, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2023

aimuz changed the title ~~wip: fix: SetLogger via klog.SetLogger will output an unexpected newline~~ fix: SetLogger via klog.SetLogger will output an unexpected newline May 17, 2023

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 17, 2023

pohly reviewed May 17, 2023

View reviewed changes

aimuz requested a review from pohly May 17, 2023 06:22

pohly suggested changes May 17, 2023

View reviewed changes

aimuz force-pushed the fix-370 branch from 5c4e670 to 1e2789b Compare May 17, 2023 10:06

pohly approved these changes May 17, 2023

View reviewed changes

skriss added a commit to projectcontour/contour that referenced this pull request Nov 7, 2023

fix unit test

00d6c3c

Fix for kubernetes/klog#378. Signed-off-by: Steve Kriss <[email protected]>

platform-engineering-bot mentioned this pull request Dec 22, 2023

Update go updates arcalot/arcaflow-engine#136

Merged

1 task

richabanker pushed a commit to richabanker/kubernetes that referenced this pull request Jan 9, 2024

dependencies: klog v2.110.1

968b735

Dropping a newline at the end of the message when using klog calls is an intentional improvement (kubernetes/klog#378)

jiahuif pushed a commit to jiahuif-forks/kubernetes that referenced this pull request Jan 23, 2024

dependencies: klog v2.110.1

70418c7

Dropping a newline at the end of the message when using klog calls is an intentional improvement (kubernetes/klog#378)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SetLogger via klog.SetLogger will output an unexpected newline #378

fix: SetLogger via klog.SetLogger will output an unexpected newline #378

aimuz commented May 11, 2023 •

edited

Loading

k8s-ci-robot commented May 11, 2023

dims commented May 11, 2023

pohly commented May 15, 2023

aimuz commented May 15, 2023

aimuz commented May 17, 2023

pohly May 17, 2023

aimuz May 17, 2023

pohly left a comment

pohly commented May 17, 2023

aimuz commented May 17, 2023

pohly left a comment

fix: SetLogger via klog.SetLogger will output an unexpected newline #378

fix: SetLogger via klog.SetLogger will output an unexpected newline #378

Conversation

aimuz commented May 11, 2023 • edited Loading

k8s-ci-robot commented May 11, 2023

dims commented May 11, 2023

pohly commented May 15, 2023

aimuz commented May 15, 2023

aimuz commented May 17, 2023

pohly May 17, 2023

Choose a reason for hiding this comment

aimuz May 17, 2023

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

pohly commented May 17, 2023

aimuz commented May 17, 2023

pohly left a comment

Choose a reason for hiding this comment

aimuz commented May 11, 2023 •

edited

Loading