Repository: kubernetes/klog
Pull request #134 (2020-04-08 22:17:00)

Fix how errors are printed
**What this PR does / why we need it**:
Fixes how errors are printed. Previously the regular `error` type would be printed as `{}`.

**Special notes for your reviewer**:
https://play.golang.org/p/iynQRn87vVZ demonstrates the bug

**Release note**:
```release-note
klogr will now print error types using their `Error()` method.
```

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/klog/pull/134#" title="Author self-approved">benmoss</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **tallclair**
You can assign the PR to them by writing `/assign @tallclair` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Fklog).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes/klog/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["tallclair"]} -->
neolit123: > TestRollover: klog_test.go:395: info wasn't created

i think this is a flake...
/retest

the change itself LGTM.

dims: /approve
/lgtm
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/klog/pull/134#" title="Author self-approved">benmoss</a>*, *<a href="https://github.com/kubernetes/klog/pull/134#issuecomment-611263433" title="LGTM">dims</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Fklog).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes/klog/blob/master/OWNERS)~~ [dims]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
benmoss: Hmm, looks like something is broken with the combination of GitHub and Travis, the build is done there but GH still reports it as pending
dims: /retest
dims: /close
k8s-ci-robot: @dims: Closed this PR.

<details>

In response to [this](https://github.com/kubernetes/klog/pull/134#issuecomment-611524175):

>/close


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
dims: /reopen
k8s-ci-robot: @dims: Reopened this PR.

<details>

In response to [this](https://github.com/kubernetes/klog/pull/134#issuecomment-611524451):

>/reopen


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 0000000..0e3fc33
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,15 @@
+language: go
+go_import_path: k8s.io/klog/v2
+dist: xenial
+go:
+  - 1.11.x
+  - 1.12.x
+  - 1.13.x
+script:
+  - go get -t -v ./...
+  - diff -u <(echo -n) <(gofmt -d .)
+  - diff -u <(echo -n) <(golint $(go list -e ./...))
+  - go tool vet . || go vet .
+  - go test -v -race ./...
+install:
+  - go get golang.org/x/lint/golint
diff --git a/klogr/klogr.go b/klogr/klogr.go
index 8e10665..944cb70 100644
--- a/klogr/klogr.go
+++ b/klogr/klogr.go
@@ -134,6 +134,11 @@ func flatten(kvList ...interface{}) string {
 }
 
 func pretty(value interface{}) string {
+	if err, ok := value.(error); ok {
+		if _, ok := value.(json.Marshaler); !ok {
+			value = err.Error()
+		}
+	}
 	jb, _ := json.Marshal(value)
 	return string(jb)
 }


Test patch:
diff --git a/klogr/klogr_test.go b/klogr/klogr_test.go
index ce59406..795eefd 100644
--- a/klogr/klogr_test.go
+++ b/klogr/klogr_test.go
@@ -2,7 +2,10 @@ package klogr
 
 import (
 	"bytes"
+	"encoding/json"
+	"errors"
 	"flag"
+	"strings"
 	"testing"
 
 	"k8s.io/klog/v2"
@@ -70,6 +73,20 @@ func TestInfo(t *testing.T) {
 			text:          "test",
 			keysAndValues: []interface{}{"akey", "avalue", "akey2"},
 			expectedOutput: ` "msg"="test" "basekey1"="basevar1" "basekey2"=null "akey"="avalue" "akey2"=null
+`,
+		},
+		"should correctly print regular error types": {
+			klogr:         New().V(0),
+			text:          "test",
+			keysAndValues: []interface{}{"err", errors.New("whoops")},
+			expectedOutput: ` "msg"="test"  "err"="whoops"
+`,
+		},
+		"should use MarshalJSON if an error type implements it": {
+			klogr:         New().V(0),
+			text:          "test",
+			keysAndValues: []interface{}{"err", &customErrorJSON{"whoops"}},
+			expectedOutput: ` "msg"="test"  "err"="WHOOPS"
 `,
 		},
 	}
@@ -95,3 +112,15 @@ func TestInfo(t *testing.T) {
 		})
 	}
 }
+
+type customErrorJSON struct {
+	s string
+}
+
+func (e *customErrorJSON) Error() string {
+	return e.s
+}
+
+func (e *customErrorJSON) MarshalJSON() ([]byte, error) {
+	return json.Marshal(strings.ToUpper(e.s))
+}