From a4f9060903da76801b2e6a905418a62a7f6b6419 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 1 Jun 2023 18:02:57 +0200 Subject: [PATCH 1/3] fix golangci-lint issues golangci-lint v1.52.2 in its default configuration pointed out several issues that are worth fixing: internal/verbosity/verbosity_test.go:26:18: Error return value of `vs.verbosity.Set` is not checked (errcheck) vs.verbosity.Set("2") ^ internal/verbosity/verbosity_test.go:41:16: Error return value of `vs.vmodule.Set` is not checked (errcheck) vs.vmodule.Set("verbosity_test=2") ^ internal/verbosity/verbosity_test.go:84:16: Error return value of `vs.vmodule.Set` is not checked (errcheck) vs.vmodule.Set(pat) ^ internal/verbosity/verbosity.go:291:2: S1017: should replace this if statement with an unconditional strings.TrimSuffix (gosimple) if strings.HasSuffix(file, ".go") { ^ textlogger/options.go:140:19: Error return value of `(flag.Value).Set` is not checked (errcheck) c.Verbosity().Set(strconv.FormatInt(int64(c.co.verbosityDefault), 10)) ^ textlogger/textlogger.go:137:26: Error return value of `l.config.co.output.Write` is not checked (errcheck) l.config.co.output.Write(b.Bytes()) ^ textlogger/textlogger.go:141:26: Error return value of `l.config.co.output.Write` is not checked (errcheck) l.config.co.output.Write(data) ^ ktesting/options.go:163:18: Error return value of `(flag.Value).Set` is not checked (errcheck) c.vstate.V().Set(strconv.FormatInt(int64(c.co.verbosityDefault), 10)) ^ klogr/calldepth-test/call_depth_main_test.go:21:22: Error return value of `flag.CommandLine.Set` is not checked (errcheck) flag.CommandLine.Set("v", "10") ^ klogr/calldepth-test/call_depth_main_test.go:22:22: Error return value of `flag.CommandLine.Set` is not checked (errcheck) flag.CommandLine.Set("skip_headers", "false") ^ klogr/calldepth-test/call_depth_main_test.go:23:22: Error return value of `flag.CommandLine.Set` is not checked (errcheck) flag.CommandLine.Set("logtostderr", "false") ^ klog.go:903:34: Error return value of `(io.Writer).Write` is not checked (errcheck) l.file[severity.InfoLog].Write(data) ^ klog.go:913:20: Error return value of `(io.Writer).Write` is not checked (errcheck) l.file[s].Write(data) ^ klog.go:917:37: Error return value of `(io.Writer).Write` is not checked (errcheck) l.file[severity.FatalLog].Write(data) ^ klog.go:952:12: Error return value of `f.Write` is not checked (errcheck) f.Write(trace) ^ klog.go:1208:13: Error return value of `file.Sync` is not checked (errcheck) file.Sync() // ignore error ^ klog_file.go:113:14: Error return value of `os.Symlink` is not checked (errcheck) os.Symlink(name, symlink) // ignore err ^ klog_test.go:328:23: Error return value of `logging.verbosity.Set` is not checked (errcheck) logging.verbosity.Set("2") ^ klog_test.go:343:21: Error return value of `logging.vmodule.Set` is not checked (errcheck) logging.vmodule.Set("klog_test=2") ^ klog_test.go:367:21: Error return value of `logging.vmodule.Set` is not checked (errcheck) logging.vmodule.Set("notthisfile=2") ^ klog_test.go:459:21: Error return value of `logging.vmodule.Set` is not checked (errcheck) logging.vmodule.Set(pat) ^ klog_test.go:807:23: Error return value of `logging.verbosity.Set` is not checked (errcheck) logging.verbosity.Set("0") ^ klog_test.go:878:9: Error return value of `fs1.Set` is not checked (errcheck) fs1.Set("log_dir", "/test1") ^ klog_test.go:879:9: Error return value of `fs1.Set` is not checked (errcheck) fs1.Set("log_file_max_size", "1") ^ klog_test.go:885:9: Error return value of `fs2.Set` is not checked (errcheck) fs2.Set("log_file_max_size", "2048") ^ klog_test.go:1173:23: Error return value of `logging.verbosity.Set` is not checked (errcheck) logging.verbosity.Set("2") ^ klogr.go:35:2: field `level` is unused (unused) level int ^ klog.go:1287:2: S1017: should replace this if statement with an unconditional strings.TrimSuffix (gosimple) if strings.HasSuffix(file, ".go") { ^ klog.go:521:2: S1001: should use copy(to, from) instead of a loop (gosimple) for i := range s.vmodule.filter { ^ klog_test.go:79:2: S1001: should copy arrays using assignment instead of using a loop (gosimple) for i, w := range writers { ^ klogr/klogr.go:118:16: Error return value of `encoder.Encode` is not checked (errcheck) encoder.Encode(value) ^ klogr/klogr_test.go:208:8: Error return value of `fs.Set` is not checked (errcheck) fs.Set("skip_headers", "true") ^ klogr/klogr.go:119:27: S1030: should use buffer.String() instead of string(buffer.Bytes()) (gosimple) return strings.TrimSpace(string(buffer.Bytes())) ^ ktesting/testinglogger_test.go:176:8: Error return value of `fs.Set` is not checked (errcheck) fs.Set("alsologtostderr", "false") ^ ktesting/testinglogger_test.go:177:8: Error return value of `fs.Set` is not checked (errcheck) fs.Set("logtostderr", "false") ^ internal/buffer/buffer.go:40:2: field `next` is unused (unused) next *Buffer ^ internal/clock/clock.go:122:9: SA1015: using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (staticcheck) return time.Tick(d) ^ exit_test.go:31:16: Error return value of `flag.Set` is not checked (errcheck) defer flag.Set("skip_headers", "false") ^ format_test.go:46:5: S1003: should use strings.Contains(str, "kind is config") instead (gosimple) if strings.Index(str, "kind is config") >= 0 { ^ --- examples/benchmarks/benchmarks_test.go | 11 +++---- examples/coexist_glog/coexist_glog.go | 5 ++-- examples/flushing/flushing_test.go | 6 ++-- examples/go.mod | 2 +- examples/klogr/main.go | 3 +- examples/log_file/usage_log_file.go | 5 ++-- examples/set_output/usage_set_output.go | 6 ++-- .../structured_logging/structured_logging.go | 5 ++-- examples/util/require/require.go | 7 +++++ exit_test.go | 13 ++++---- format_test.go | 2 +- internal/buffer/buffer.go | 3 +- internal/clock/clock.go | 21 ++----------- internal/clock/testing/fake_clock.go | 7 ----- internal/test/require/require.go | 26 ++++++++++++++++ internal/verbosity/verbosity.go | 4 +-- internal/verbosity/verbosity_test.go | 8 +++-- klog.go | 30 ++++++++----------- klog_file.go | 4 +-- klog_test.go | 23 +++++++------- klogr.go | 1 - klogr/calldepth-test/call_depth_main_test.go | 11 +++---- klogr/klogr.go | 6 ++-- klogr/klogr_test.go | 3 +- ktesting/options.go | 3 +- ktesting/testinglogger_test.go | 9 +++--- textlogger/options.go | 3 +- textlogger/textlogger.go | 4 +-- 28 files changed, 126 insertions(+), 105 deletions(-) create mode 100644 examples/util/require/require.go create mode 100644 internal/test/require/require.go diff --git a/examples/benchmarks/benchmarks_test.go b/examples/benchmarks/benchmarks_test.go index a343efe08..ecb57ca9b 100644 --- a/examples/benchmarks/benchmarks_test.go +++ b/examples/benchmarks/benchmarks_test.go @@ -27,6 +27,7 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" + "k8s.io/klog/examples/util/require" "k8s.io/klog/v2" ) @@ -38,11 +39,11 @@ func init() { // klog gets configured so that it writes to a single output file that // will be set during tests with SetOutput. klog.InitFlags(nil) - flag.Set("v", fmt.Sprintf("%d", verbosityThreshold)) - flag.Set("log_file", "/dev/null") - flag.Set("logtostderr", "false") - flag.Set("alsologtostderr", "false") - flag.Set("stderrthreshold", "10") + require.NoError(flag.Set("v", fmt.Sprintf("%d", verbosityThreshold))) + require.NoError(flag.Set("log_file", "/dev/null")) + require.NoError(flag.Set("logtostderr", "false")) + require.NoError(flag.Set("alsologtostderr", "false")) + require.NoError(flag.Set("stderrthreshold", "10")) } type testcase struct { diff --git a/examples/coexist_glog/coexist_glog.go b/examples/coexist_glog/coexist_glog.go index 960a9b931..9f3fd057f 100644 --- a/examples/coexist_glog/coexist_glog.go +++ b/examples/coexist_glog/coexist_glog.go @@ -4,11 +4,12 @@ import ( "flag" "github.com/golang/glog" + "k8s.io/klog/examples/util/require" "k8s.io/klog/v2" ) func main() { - flag.Set("alsologtostderr", "true") + require.NoError(flag.Set("alsologtostderr", "true")) flag.Parse() klogFlags := flag.NewFlagSet("klog", flag.ExitOnError) @@ -19,7 +20,7 @@ func main() { f2 := klogFlags.Lookup(f1.Name) if f2 != nil { value := f1.Value.String() - f2.Value.Set(value) + require.NoError(f2.Value.Set(value)) } }) diff --git a/examples/flushing/flushing_test.go b/examples/flushing/flushing_test.go index 3d46fbcab..18a4a4572 100644 --- a/examples/flushing/flushing_test.go +++ b/examples/flushing/flushing_test.go @@ -5,6 +5,8 @@ import ( "testing" "go.uber.org/goleak" + + "k8s.io/klog/examples/util/require" "k8s.io/klog/v2" ) @@ -13,8 +15,8 @@ func main() { // By default klog writes to stderr. Setting logtostderr to false makes klog // write to a log file. - flag.Set("logtostderr", "false") - flag.Set("log_file", "myfile.log") + require.NoError(flag.Set("logtostderr", "false")) + require.NoError(flag.Set("log_file", "myfile.log")) flag.Parse() // Info writes the first log message. When the first log file is created, diff --git a/examples/go.mod b/examples/go.mod index b7750acfb..4fee663f1 100644 --- a/examples/go.mod +++ b/examples/go.mod @@ -1,4 +1,4 @@ -module example +module k8s.io/klog/examples go 1.13 diff --git a/examples/klogr/main.go b/examples/klogr/main.go index 5de2b5f52..db5539aa5 100644 --- a/examples/klogr/main.go +++ b/examples/klogr/main.go @@ -3,6 +3,7 @@ package main import ( "flag" + "k8s.io/klog/examples/util/require" "k8s.io/klog/v2" "k8s.io/klog/v2/klogr" ) @@ -17,7 +18,7 @@ func (e myError) Error() string { func main() { klog.InitFlags(nil) - flag.Set("v", "3") + require.NoError(flag.Set("v", "3")) flag.Parse() log := klogr.New().WithName("MyName").WithValues("user", "you") log.Info("hello", "val1", 1, "val2", map[string]int{"k": 1}) diff --git a/examples/log_file/usage_log_file.go b/examples/log_file/usage_log_file.go index bff48e043..4badabc6f 100644 --- a/examples/log_file/usage_log_file.go +++ b/examples/log_file/usage_log_file.go @@ -3,6 +3,7 @@ package main import ( "flag" + "k8s.io/klog/examples/util/require" "k8s.io/klog/v2" ) @@ -10,8 +11,8 @@ func main() { klog.InitFlags(nil) // By default klog writes to stderr. Setting logtostderr to false makes klog // write to a log file. - flag.Set("logtostderr", "false") - flag.Set("log_file", "myfile.log") + require.NoError(flag.Set("logtostderr", "false")) + require.NoError(flag.Set("log_file", "myfile.log")) flag.Parse() klog.Info("nice to meet you") klog.Flush() diff --git a/examples/set_output/usage_set_output.go b/examples/set_output/usage_set_output.go index 3cdd3884f..99ad8d01a 100644 --- a/examples/set_output/usage_set_output.go +++ b/examples/set_output/usage_set_output.go @@ -4,13 +4,15 @@ import ( "bytes" "flag" "fmt" + + "k8s.io/klog/examples/util/require" "k8s.io/klog/v2" ) func main() { klog.InitFlags(nil) - flag.Set("logtostderr", "false") - flag.Set("alsologtostderr", "false") + require.NoError(flag.Set("logtostderr", "false")) + require.NoError(flag.Set("alsologtostderr", "false")) flag.Parse() buf := new(bytes.Buffer) diff --git a/examples/structured_logging/structured_logging.go b/examples/structured_logging/structured_logging.go index 6e80bd936..b2ae83476 100644 --- a/examples/structured_logging/structured_logging.go +++ b/examples/structured_logging/structured_logging.go @@ -25,8 +25,9 @@ func main() { flag.Parse() someData := MyStruct{ - Name: "hello", - Data: "world", + Name: "hello", + Data: "world", + internal: 42, } longData := MyStruct{ diff --git a/examples/util/require/require.go b/examples/util/require/require.go new file mode 100644 index 000000000..0c458553b --- /dev/null +++ b/examples/util/require/require.go @@ -0,0 +1,7 @@ +package require + +func NoError(err error) { + if err != nil { + panic(err) + } +} diff --git a/exit_test.go b/exit_test.go index e295eb8e8..8c0a03759 100644 --- a/exit_test.go +++ b/exit_test.go @@ -27,12 +27,15 @@ func ExampleFlushAndExit() { var fs flag.FlagSet klog.InitFlags(&fs) - fs.Set("skip_headers", "true") - defer flag.Set("skip_headers", "false") - fs.Set("logtostderr", "false") - defer fs.Set("logtostderr", "true") + state := klog.CaptureState() + defer state.Restore() + if err := fs.Set("skip_headers", "true"); err != nil { + panic(err) + } + if err := fs.Set("logtostderr", "false"); err != nil { + panic(err) + } klog.SetOutput(os.Stdout) - defer klog.SetOutput(nil) klog.OsExit = func(exitCode int) { fmt.Printf("os.Exit(%d)\n", exitCode) } diff --git a/format_test.go b/format_test.go index cd1914581..490c96630 100644 --- a/format_test.go +++ b/format_test.go @@ -43,7 +43,7 @@ func TestFormat(t *testing.T) { `, klog.Format(obj).(fmt.Stringer).String(), "Format(config).String()") // fmt.Sprintf would call String if it was available. str := fmt.Sprintf("%s", klog.Format(obj).(logr.Marshaler).MarshalLog()) - if strings.Index(str, "kind is config") >= 0 { + if strings.Contains(str, "kind is config") { t.Errorf("fmt.Sprintf called TypeMeta.String for klog.Format(obj).MarshalLog():\n%s", str) } diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index f325ded5e..475e1bf3d 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -36,8 +36,7 @@ var ( // use. It also provides some helper methods for output formatting. type Buffer struct { bytes.Buffer - Tmp [64]byte // temporary byte array for creating headers. - next *Buffer + Tmp [64]byte // temporary byte array for creating headers. } var buffers = sync.Pool{ diff --git a/internal/clock/clock.go b/internal/clock/clock.go index b8b6af5c8..cc11bb480 100644 --- a/internal/clock/clock.go +++ b/internal/clock/clock.go @@ -39,16 +39,6 @@ type Clock interface { // Sleep sleeps for the provided duration d. // Consider making the sleep interruptible by using 'select' on a context channel and a timer channel. Sleep(d time.Duration) - // Tick returns the channel of a new Ticker. - // This method does not allow to free/GC the backing ticker. Use - // NewTicker from WithTicker instead. - Tick(d time.Duration) <-chan time.Time -} - -// WithTicker allows for injecting fake or real clocks into code that -// needs to do arbitrary things based on time. -type WithTicker interface { - Clock // NewTicker returns a new Ticker. NewTicker(time.Duration) Ticker } @@ -66,7 +56,7 @@ type WithDelayedExecution interface { // WithTickerAndDelayedExecution allows for injecting fake or real clocks // into code that needs Ticker and AfterFunc functionality type WithTickerAndDelayedExecution interface { - WithTicker + Clock // AfterFunc executes f in its own goroutine after waiting // for d duration and returns a Timer whose channel can be // closed by calling Stop() on the Timer. @@ -79,7 +69,7 @@ type Ticker interface { Stop() } -var _ = WithTicker(RealClock{}) +var _ Clock = RealClock{} // RealClock really calls time.Now() type RealClock struct{} @@ -115,13 +105,6 @@ func (RealClock) AfterFunc(d time.Duration, f func()) Timer { } } -// Tick is the same as time.Tick(d) -// This method does not allow to free/GC the backing ticker. Use -// NewTicker instead. -func (RealClock) Tick(d time.Duration) <-chan time.Time { - return time.Tick(d) -} - // NewTicker returns a new Ticker. func (RealClock) NewTicker(d time.Duration) Ticker { return &realTicker{ diff --git a/internal/clock/testing/fake_clock.go b/internal/clock/testing/fake_clock.go index a91114a58..392f81e04 100644 --- a/internal/clock/testing/fake_clock.go +++ b/internal/clock/testing/fake_clock.go @@ -25,7 +25,6 @@ import ( var ( _ = clock.PassiveClock(&FakePassiveClock{}) - _ = clock.WithTicker(&FakeClock{}) _ = clock.Clock(&IntervalClock{}) ) @@ -275,12 +274,6 @@ func (*IntervalClock) AfterFunc(d time.Duration, f func()) clock.Timer { panic("IntervalClock doesn't implement AfterFunc") } -// Tick is unimplemented, will panic. -// TODO: make interval clock use FakeClock so this can be implemented. -func (*IntervalClock) Tick(d time.Duration) <-chan time.Time { - panic("IntervalClock doesn't implement Tick") -} - // NewTicker has no implementation yet and is omitted. // TODO: make interval clock use FakeClock so this can be implemented. func (*IntervalClock) NewTicker(d time.Duration) clock.Ticker { diff --git a/internal/test/require/require.go b/internal/test/require/require.go new file mode 100644 index 000000000..93a869260 --- /dev/null +++ b/internal/test/require/require.go @@ -0,0 +1,26 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package require + +import "testing" + +func NoError(tb testing.TB, err error) { + if err != nil { + tb.Helper() + tb.Fatalf("Unexpected error: %v", err) + } +} diff --git a/internal/verbosity/verbosity.go b/internal/verbosity/verbosity.go index 309cdfaa9..40ec27d87 100644 --- a/internal/verbosity/verbosity.go +++ b/internal/verbosity/verbosity.go @@ -288,9 +288,7 @@ func (vs *VState) setV(pc uintptr) Level { fn := runtime.FuncForPC(pc) file, _ := fn.FileLine(pc) // The file is something like /a/b/c/d.go. We want just the d. - if strings.HasSuffix(file, ".go") { - file = file[:len(file)-3] - } + file = strings.TrimSuffix(file, ".go") if slash := strings.LastIndex(file, "/"); slash >= 0 { file = file[slash+1:] } diff --git a/internal/verbosity/verbosity_test.go b/internal/verbosity/verbosity_test.go index 766147b27..5735d486c 100644 --- a/internal/verbosity/verbosity_test.go +++ b/internal/verbosity/verbosity_test.go @@ -19,11 +19,13 @@ package verbosity import ( "testing" + + "k8s.io/klog/v2/internal/test/require" ) func TestV(t *testing.T) { vs := New() - vs.verbosity.Set("2") + require.NoError(t, vs.verbosity.Set("2")) depth := 0 if !vs.Enabled(1, depth) { t.Error("not enabled for 1") @@ -38,7 +40,7 @@ func TestV(t *testing.T) { func TestVmoduleOn(t *testing.T) { vs := New() - vs.vmodule.Set("verbosity_test=2") + require.NoError(t, vs.vmodule.Set("verbosity_test=2")) depth := 0 if !vs.Enabled(1, depth) { t.Error("not enabled for 1") @@ -81,7 +83,7 @@ var vGlobs = map[string]bool{ // Test that vmodule globbing works as advertised. func testVmoduleGlob(pat string, match bool, t *testing.T) { vs := New() - vs.vmodule.Set(pat) + require.NoError(t, vs.vmodule.Set(pat)) depth := 0 actual := vs.Enabled(2, depth) if actual != match { diff --git a/klog.go b/klog.go index c18576e3e..e50b1f2fa 100644 --- a/klog.go +++ b/klog.go @@ -518,9 +518,7 @@ type settings struct { func (s settings) deepCopy() settings { // vmodule is a slice and would be shared, so we have copy it. filter := make([]modulePat, len(s.vmodule.filter)) - for i := range s.vmodule.filter { - filter[i] = s.vmodule.filter[i] - } + copy(filter, s.vmodule.filter) s.vmodule.filter = filter if s.logger != nil { @@ -900,7 +898,7 @@ func (l *loggingT) output(s severity.Severity, logger *logWriter, buf *buffer.Bu l.exit(err) } } - l.file[severity.InfoLog].Write(data) + _, _ = l.file[severity.InfoLog].Write(data) } else { if l.file[s] == nil { if err := l.createFiles(s); err != nil { @@ -910,20 +908,20 @@ func (l *loggingT) output(s severity.Severity, logger *logWriter, buf *buffer.Bu } if l.oneOutput { - l.file[s].Write(data) + _, _ = l.file[s].Write(data) } else { switch s { case severity.FatalLog: - l.file[severity.FatalLog].Write(data) + _, _ = l.file[severity.FatalLog].Write(data) fallthrough case severity.ErrorLog: - l.file[severity.ErrorLog].Write(data) + _, _ = l.file[severity.ErrorLog].Write(data) fallthrough case severity.WarningLog: - l.file[severity.WarningLog].Write(data) + _, _ = l.file[severity.WarningLog].Write(data) fallthrough case severity.InfoLog: - l.file[severity.InfoLog].Write(data) + _, _ = l.file[severity.InfoLog].Write(data) } } } @@ -949,7 +947,7 @@ func (l *loggingT) output(s severity.Severity, logger *logWriter, buf *buffer.Bu logExitFunc = func(error) {} // If we get a write error, we'll still exit below. for log := severity.FatalLog; log >= severity.InfoLog; log-- { if f := l.file[log]; f != nil { // Can be nil if -logtostderr is set. - f.Write(trace) + _, _ = f.Write(trace) } } l.mu.Unlock() @@ -1105,7 +1103,7 @@ const flushInterval = 5 * time.Second // flushDaemon periodically flushes the log file buffers. type flushDaemon struct { mu sync.Mutex - clock clock.WithTicker + clock clock.Clock flush func() stopC chan struct{} stopDone chan struct{} @@ -1113,7 +1111,7 @@ type flushDaemon struct { // newFlushDaemon returns a new flushDaemon. If the passed clock is nil, a // clock.RealClock is used. -func newFlushDaemon(flush func(), tickClock clock.WithTicker) *flushDaemon { +func newFlushDaemon(flush func(), tickClock clock.Clock) *flushDaemon { if tickClock == nil { tickClock = clock.RealClock{} } @@ -1204,8 +1202,8 @@ func (l *loggingT) flushAll() { for s := severity.FatalLog; s >= severity.InfoLog; s-- { file := l.file[s] if file != nil { - file.Flush() // ignore error - file.Sync() // ignore error + _ = file.Flush() // ignore error + _ = file.Sync() // ignore error } } if logging.loggerOptions.flush != nil { @@ -1284,9 +1282,7 @@ func (l *loggingT) setV(pc uintptr) Level { fn := runtime.FuncForPC(pc) file, _ := fn.FileLine(pc) // The file is something like /a/b/c/d.go. We want just the d. - if strings.HasSuffix(file, ".go") { - file = file[:len(file)-3] - } + file = strings.TrimSuffix(file, ".go") if slash := strings.LastIndex(file, "/"); slash >= 0 { file = file[slash+1:] } diff --git a/klog_file.go b/klog_file.go index 1025d644f..8bee16204 100644 --- a/klog_file.go +++ b/klog_file.go @@ -109,8 +109,8 @@ func create(tag string, t time.Time, startup bool) (f *os.File, filename string, f, err := openOrCreate(fname, startup) if err == nil { symlink := filepath.Join(dir, link) - os.Remove(symlink) // ignore err - os.Symlink(name, symlink) // ignore err + _ = os.Remove(symlink) // ignore err + _ = os.Symlink(name, symlink) // ignore err return f, fname, nil } lastErr = err diff --git a/klog_test.go b/klog_test.go index 7b6ebdb6f..a9ddc5a77 100644 --- a/klog_test.go +++ b/klog_test.go @@ -40,6 +40,7 @@ import ( testingclock "k8s.io/klog/v2/internal/clock/testing" "k8s.io/klog/v2/internal/severity" "k8s.io/klog/v2/internal/test" + "k8s.io/klog/v2/internal/test/require" ) // TODO: This test package should be refactored so that tests cannot @@ -76,9 +77,7 @@ func (l *loggingT) swap(writers [severity.NumSeverity]flushSyncWriter) (old [sev l.mu.Lock() defer l.mu.Unlock() old = l.file - for i, w := range writers { - logging.file[i] = w - } + logging.file = writers return } @@ -325,7 +324,7 @@ func TestV(t *testing.T) { defer CaptureState().Restore() setFlags() defer logging.swap(logging.newBuffers()) - logging.verbosity.Set("2") + require.NoError(t, logging.verbosity.Set("2")) V(2).Info("test") if !contains(severity.InfoLog, "I", t) { t.Errorf("Info has wrong character: %q", contents(severity.InfoLog)) @@ -340,7 +339,7 @@ func TestVmoduleOn(t *testing.T) { defer CaptureState().Restore() setFlags() defer logging.swap(logging.newBuffers()) - logging.vmodule.Set("klog_test=2") + require.NoError(t, logging.vmodule.Set("klog_test=2")) if !V(1).Enabled() { t.Error("V not enabled for 1") } @@ -364,7 +363,7 @@ func TestVmoduleOff(t *testing.T) { defer CaptureState().Restore() setFlags() defer logging.swap(logging.newBuffers()) - logging.vmodule.Set("notthisfile=2") + require.NoError(t, logging.vmodule.Set("notthisfile=2")) for i := 1; i <= 3; i++ { if V(Level(i)).Enabled() { t.Errorf("V enabled for %d", i) @@ -456,7 +455,7 @@ func testVmoduleGlob(pat string, match bool, t *testing.T) { defer CaptureState().Restore() setFlags() defer logging.swap(logging.newBuffers()) - logging.vmodule.Set(pat) + require.NoError(t, logging.vmodule.Set(pat)) if V(2).Enabled() != match { t.Errorf("incorrect match for %q: got %#v expected %#v", pat, V(2), match) } @@ -804,7 +803,7 @@ func BenchmarkLogs(b *testing.B) { } defer os.Remove(testFile.Name()) - logging.verbosity.Set("0") + require.NoError(b, logging.verbosity.Set("0")) logging.toStderr = false logging.alsoToStderr = false logging.stderrThreshold = severityValue{ @@ -875,14 +874,14 @@ func TestInitFlags(t *testing.T) { fs1 := flag.NewFlagSet("test1", flag.PanicOnError) InitFlags(fs1) - fs1.Set("log_dir", "/test1") - fs1.Set("log_file_max_size", "1") + require.NoError(t, fs1.Set("log_dir", "/test1")) + require.NoError(t, fs1.Set("log_file_max_size", "1")) fs2 := flag.NewFlagSet("test2", flag.PanicOnError) InitFlags(fs2) if logging.logDir != "/test1" { t.Fatalf("Expected log_dir to be %q, got %q", "/test1", logging.logDir) } - fs2.Set("log_file_max_size", "2048") + require.NoError(t, fs2.Set("log_file_max_size", "2048")) if logging.logFileMaxSizeMB != 2048 { t.Fatal("Expected log_file_max_size to be 2048") } @@ -1170,7 +1169,7 @@ second value line`}, }, } - logging.verbosity.Set("2") + require.NoError(t, logging.verbosity.Set("2")) for l := Level(0); l < Level(4); l++ { for _, data := range testDataInfo { diff --git a/klogr.go b/klogr.go index 15de00e21..784f7de13 100644 --- a/klogr.go +++ b/klogr.go @@ -32,7 +32,6 @@ func NewKlogr() Logger { // klogger is a subset of klogr/klogr.go. It had to be copied to break an // import cycle (klogr wants to use klog, and klog wants to use klogr). type klogger struct { - level int callDepth int prefix string values []interface{} diff --git a/klogr/calldepth-test/call_depth_main_test.go b/klogr/calldepth-test/call_depth_main_test.go index 6d7e59f81..83fe4309e 100644 --- a/klogr/calldepth-test/call_depth_main_test.go +++ b/klogr/calldepth-test/call_depth_main_test.go @@ -13,16 +13,17 @@ import ( "testing" "k8s.io/klog/v2" + "k8s.io/klog/v2/internal/test/require" "k8s.io/klog/v2/klogr" ) func TestCallDepth(t *testing.T) { klog.InitFlags(nil) - flag.CommandLine.Set("v", "10") - flag.CommandLine.Set("skip_headers", "false") - flag.CommandLine.Set("logtostderr", "false") - flag.CommandLine.Set("alsologtostderr", "false") - flag.CommandLine.Set("stderrthreshold", "10") + require.NoError(t, flag.CommandLine.Set("v", "10")) + require.NoError(t, flag.CommandLine.Set("skip_headers", "false")) + require.NoError(t, flag.CommandLine.Set("logtostderr", "false")) + require.NoError(t, flag.CommandLine.Set("alsologtostderr", "false")) + require.NoError(t, flag.CommandLine.Set("stderrthreshold", "10")) flag.Parse() t.Run("call-depth", func(t *testing.T) { diff --git a/klogr/klogr.go b/klogr/klogr.go index db5b5c789..d80e44fdc 100644 --- a/klogr/klogr.go +++ b/klogr/klogr.go @@ -115,8 +115,10 @@ func pretty(value interface{}) string { buffer := &bytes.Buffer{} encoder := json.NewEncoder(buffer) encoder.SetEscapeHTML(false) - encoder.Encode(value) - return strings.TrimSpace(string(buffer.Bytes())) + if err := encoder.Encode(value); err != nil { + return fmt.Sprintf("<>", err) + } + return strings.TrimSpace(buffer.String()) } func (l *klogger) Info(level int, msg string, kvList ...interface{}) { diff --git a/klogr/klogr_test.go b/klogr/klogr_test.go index 53b9e450d..621017d0b 100644 --- a/klogr/klogr_test.go +++ b/klogr/klogr_test.go @@ -8,6 +8,7 @@ import ( "testing" "k8s.io/klog/v2" + "k8s.io/klog/v2/internal/test/require" "k8s.io/klog/v2/test" "github.com/go-logr/logr" @@ -205,7 +206,7 @@ func testOutput(t *testing.T, format string) { func TestOutput(t *testing.T) { fs := test.InitKlog(t) - fs.Set("skip_headers", "true") + require.NoError(t, fs.Set("skip_headers", "true")) formats := []string{ formatNew, diff --git a/ktesting/options.go b/ktesting/options.go index d039c40b0..a1b4f6af0 100644 --- a/ktesting/options.go +++ b/ktesting/options.go @@ -160,7 +160,8 @@ func NewConfig(opts ...ConfigOption) *Config { } c.vstate = verbosity.New() - c.vstate.V().Set(strconv.FormatInt(int64(c.co.verbosityDefault), 10)) + // Cannot fail for this input. + _ = c.vstate.V().Set(strconv.FormatInt(int64(c.co.verbosityDefault), 10)) return c } diff --git a/ktesting/testinglogger_test.go b/ktesting/testinglogger_test.go index 6de1834d4..1ec709b50 100644 --- a/ktesting/testinglogger_test.go +++ b/ktesting/testinglogger_test.go @@ -18,6 +18,7 @@ import ( "testing" "k8s.io/klog/v2" + "k8s.io/klog/v2/internal/test/require" "k8s.io/klog/v2/ktesting" ) @@ -173,10 +174,10 @@ func TestStop(t *testing.T) { var output bytes.Buffer var fs flag.FlagSet klog.InitFlags(&fs) - fs.Set("alsologtostderr", "false") - fs.Set("logtostderr", "false") - fs.Set("stderrthreshold", "FATAL") - fs.Set("one_output", "true") + require.NoError(t, fs.Set("alsologtostderr", "false")) + require.NoError(t, fs.Set("logtostderr", "false")) + require.NoError(t, fs.Set("stderrthreshold", "FATAL")) + require.NoError(t, fs.Set("one_output", "true")) klog.SetOutput(&output) var logger klog.Logger diff --git a/textlogger/options.go b/textlogger/options.go index db4cbebdc..83eeef407 100644 --- a/textlogger/options.go +++ b/textlogger/options.go @@ -137,7 +137,8 @@ func NewConfig(opts ...ConfigOption) *Config { opt(&c.co) } - c.Verbosity().Set(strconv.FormatInt(int64(c.co.verbosityDefault), 10)) + // Cannot fail for this input. + _ = c.Verbosity().Set(strconv.FormatInt(int64(c.co.verbosityDefault), 10)) return c } diff --git a/textlogger/textlogger.go b/textlogger/textlogger.go index 3c0e079ad..857c76922 100644 --- a/textlogger/textlogger.go +++ b/textlogger/textlogger.go @@ -134,11 +134,11 @@ func (l *tlogger) print(err error, s severity.Severity, msg string, kvList []int if b.Len() == 0 || b.Bytes()[b.Len()-1] != '\n' { b.WriteByte('\n') } - l.config.co.output.Write(b.Bytes()) + _, _ = l.config.co.output.Write(b.Bytes()) } func (l *tlogger) WriteKlogBuffer(data []byte) { - l.config.co.output.Write(data) + _, _ = l.config.co.output.Write(data) } // WithName returns a new logr.Logger with the specified name appended. klogr From ef25537ae5d41941682b835688b2e46fd2ec4c1f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 1 Jun 2023 18:33:26 +0200 Subject: [PATCH 2/3] fix revive issues In golangci-lint and upstream, revive has replaced golint because golint is no longer maintained. revive finds some more issues that need to be fixed before it can be used: ktesting/testinglogger.go:81:20: unused-parameter: parameter 'args' seems to be unused, consider removing or renaming it as _ (revive) func (n NopTL) Log(args ...interface{}) {} ^ klogr/klogr_test.go:23:2: redefines-builtin-id: redefinition of the built-in function new (revive) new := func() logr.Logger { switch format { case formatNew: return New() case formatDefault: return NewWithOptions() default: return NewWithOptions(WithFormat(Format(format))) } } internal/clock/testing/fake_clock.go:261:29: unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _ (revive) func (*IntervalClock) After(d time.Duration) <-chan time.Time { ^ internal/clock/testing/fake_clock.go:267:32: unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _ (revive) func (*IntervalClock) NewTimer(d time.Duration) clock.Timer { ^ internal/clock/testing/fake_clock.go:273:33: unused-parameter: parameter 'd' seems to be unused, consider removing or renaming it as _ (revive) func (*IntervalClock) AfterFunc(d time.Duration, f func()) clock.Timer { ^ textlogger/textlogger.go:148:2: redefines-builtin-id: redefinition of the built-in function new (revive) new := *l ^ textlogger/textlogger.go:157:2: redefines-builtin-id: redefinition of the built-in function new (revive) new := *l ^ textlogger/textlogger.go:91:24: unused-parameter: parameter 'level' seems to be unused, consider removing or renaming it as _ (revive) func (l *tlogger) Info(level int, msg string, kvList ...interface{}) { ^ klog_test.go:2204:2: redefines-builtin-id: redefinition of the built-in function copy (revive) copy := settings.deepCopy() ^ klog_test.go:95:48: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) func contains(s severity.Severity, str string, t *testing.T) bool { ^ klog_test.go:378:28: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) func TestSetOutputDataRace(t *testing.T) { ^ klog_test.go:1807:25: unused-parameter: parameter 'level' seems to be unused, consider removing or renaming it as _ (revive) func (l *testLogr) Info(level int, msg string, keysAndValues ...interface{}) { ^ klog_test.go:1828:25: unused-parameter: parameter 'info' seems to be unused, consider removing or renaming it as _ (revive) func (l *testLogr) Init(info logr.RuntimeInfo) {} ^ klog_test.go:1829:28: unused-parameter: parameter 'level' seems to be unused, consider removing or renaming it as _ (revive) func (l *testLogr) Enabled(level int) bool { return true } ^ klog_test.go:1833:34: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive) func (l *testLogr) WithCallDepth(depth int) logr.LogSink { return l } ^ --- examples/output_test/output_test.go | 4 +- internal/clock/testing/fake_clock.go | 10 ++-- klog_test.go | 70 ++++++++++++++-------------- klogr/klogr_test.go | 32 ++++++------- ktesting/testinglogger.go | 4 +- textlogger/textlogger.go | 16 +++---- 6 files changed, 69 insertions(+), 67 deletions(-) diff --git a/examples/output_test/output_test.go b/examples/output_test/output_test.go index 872612f72..e9a79d4c4 100644 --- a/examples/output_test/output_test.go +++ b/examples/output_test/output_test.go @@ -34,7 +34,9 @@ import ( "k8s.io/klog/v2/textlogger" ) -func newLogger(out io.Writer, v int, vmodule string) logr.Logger { +// newLogger is a test.OutputConfig.NewLogger callback which creates a zapr +// logger. The vmodule parameter is ignored because zapr does not support that. +func newLogger(out io.Writer, v int, _ string) logr.Logger { return newZaprLogger(out, v) } diff --git a/internal/clock/testing/fake_clock.go b/internal/clock/testing/fake_clock.go index 392f81e04..61d26a5c0 100644 --- a/internal/clock/testing/fake_clock.go +++ b/internal/clock/testing/fake_clock.go @@ -258,30 +258,30 @@ func (i *IntervalClock) Since(ts time.Time) time.Duration { // After is unimplemented, will panic. // TODO: make interval clock use FakeClock so this can be implemented. -func (*IntervalClock) After(d time.Duration) <-chan time.Time { +func (*IntervalClock) After(time.Duration) <-chan time.Time { panic("IntervalClock doesn't implement After") } // NewTimer is unimplemented, will panic. // TODO: make interval clock use FakeClock so this can be implemented. -func (*IntervalClock) NewTimer(d time.Duration) clock.Timer { +func (*IntervalClock) NewTimer(time.Duration) clock.Timer { panic("IntervalClock doesn't implement NewTimer") } // AfterFunc is unimplemented, will panic. // TODO: make interval clock use FakeClock so this can be implemented. -func (*IntervalClock) AfterFunc(d time.Duration, f func()) clock.Timer { +func (*IntervalClock) AfterFunc(time.Duration, func()) clock.Timer { panic("IntervalClock doesn't implement AfterFunc") } // NewTicker has no implementation yet and is omitted. // TODO: make interval clock use FakeClock so this can be implemented. -func (*IntervalClock) NewTicker(d time.Duration) clock.Ticker { +func (*IntervalClock) NewTicker(time.Duration) clock.Ticker { panic("IntervalClock doesn't implement NewTicker") } // Sleep is unimplemented, will panic. -func (*IntervalClock) Sleep(d time.Duration) { +func (*IntervalClock) Sleep(time.Duration) { panic("IntervalClock doesn't implement Sleep") } diff --git a/klog_test.go b/klog_test.go index a9ddc5a77..4528bc2a4 100644 --- a/klog_test.go +++ b/klog_test.go @@ -92,7 +92,7 @@ func contents(s severity.Severity) string { } // contains reports whether the string is contained in the log. -func contains(s severity.Severity, str string, t *testing.T) bool { +func contains(s severity.Severity, str string) bool { return strings.Contains(contents(s), str) } @@ -108,10 +108,10 @@ func TestInfo(t *testing.T) { setFlags() defer logging.swap(logging.newBuffers()) Info("test") - if !contains(severity.InfoLog, "I", t) { + if !contains(severity.InfoLog, "I") { t.Errorf("Info has wrong character: %q", contents(severity.InfoLog)) } - if !contains(severity.InfoLog, "test", t) { + if !contains(severity.InfoLog, "test") { t.Error("Info failed") } } @@ -181,10 +181,10 @@ func TestStandardLog(t *testing.T) { setFlags() defer logging.swap(logging.newBuffers()) stdLog.Print("test") - if !contains(severity.InfoLog, "I", t) { + if !contains(severity.InfoLog, "I") { t.Errorf("Info has wrong character: %q", contents(severity.InfoLog)) } - if !contains(severity.InfoLog, "test", t) { + if !contains(severity.InfoLog, "test") { t.Error("Info failed") } } @@ -239,17 +239,17 @@ func TestError(t *testing.T) { setFlags() defer logging.swap(logging.newBuffers()) Error("test") - if !contains(severity.ErrorLog, "E", t) { + if !contains(severity.ErrorLog, "E") { t.Errorf("Error has wrong character: %q", contents(severity.ErrorLog)) } - if !contains(severity.ErrorLog, "test", t) { + if !contains(severity.ErrorLog, "test") { t.Error("Error failed") } str := contents(severity.ErrorLog) - if !contains(severity.WarningLog, str, t) { + if !contains(severity.WarningLog, str) { t.Error("Warning failed") } - if !contains(severity.InfoLog, str, t) { + if !contains(severity.InfoLog, str) { t.Error("Info failed") } } @@ -263,17 +263,17 @@ func TestErrorWithOneOutput(t *testing.T) { logging.oneOutput = true defer logging.swap(logging.newBuffers()) Error("test") - if !contains(severity.ErrorLog, "E", t) { + if !contains(severity.ErrorLog, "E") { t.Errorf("Error has wrong character: %q", contents(severity.ErrorLog)) } - if !contains(severity.ErrorLog, "test", t) { + if !contains(severity.ErrorLog, "test") { t.Error("Error failed") } str := contents(severity.ErrorLog) - if contains(severity.WarningLog, str, t) { + if contains(severity.WarningLog, str) { t.Error("Warning failed") } - if contains(severity.InfoLog, str, t) { + if contains(severity.InfoLog, str) { t.Error("Info failed") } } @@ -286,14 +286,14 @@ func TestWarning(t *testing.T) { setFlags() defer logging.swap(logging.newBuffers()) Warning("test") - if !contains(severity.WarningLog, "W", t) { + if !contains(severity.WarningLog, "W") { t.Errorf("Warning has wrong character: %q", contents(severity.WarningLog)) } - if !contains(severity.WarningLog, "test", t) { + if !contains(severity.WarningLog, "test") { t.Error("Warning failed") } str := contents(severity.WarningLog) - if !contains(severity.InfoLog, str, t) { + if !contains(severity.InfoLog, str) { t.Error("Info failed") } } @@ -307,14 +307,14 @@ func TestWarningWithOneOutput(t *testing.T) { logging.oneOutput = true defer logging.swap(logging.newBuffers()) Warning("test") - if !contains(severity.WarningLog, "W", t) { + if !contains(severity.WarningLog, "W") { t.Errorf("Warning has wrong character: %q", contents(severity.WarningLog)) } - if !contains(severity.WarningLog, "test", t) { + if !contains(severity.WarningLog, "test") { t.Error("Warning failed") } str := contents(severity.WarningLog) - if contains(severity.InfoLog, str, t) { + if contains(severity.InfoLog, str) { t.Error("Info failed") } } @@ -326,10 +326,10 @@ func TestV(t *testing.T) { defer logging.swap(logging.newBuffers()) require.NoError(t, logging.verbosity.Set("2")) V(2).Info("test") - if !contains(severity.InfoLog, "I", t) { + if !contains(severity.InfoLog, "I") { t.Errorf("Info has wrong character: %q", contents(severity.InfoLog)) } - if !contains(severity.InfoLog, "test", t) { + if !contains(severity.InfoLog, "test") { t.Error("Info failed") } } @@ -350,10 +350,10 @@ func TestVmoduleOn(t *testing.T) { t.Error("V enabled for 3") } V(2).Info("test") - if !contains(severity.InfoLog, "I", t) { + if !contains(severity.InfoLog, "I") { t.Errorf("Info has wrong character: %q", contents(severity.InfoLog)) } - if !contains(severity.InfoLog, "test", t) { + if !contains(severity.InfoLog, "test") { t.Error("Info failed") } } @@ -375,7 +375,7 @@ func TestVmoduleOff(t *testing.T) { } } -func TestSetOutputDataRace(t *testing.T) { +func TestSetOutputDataRace(*testing.T) { defer CaptureState().Restore() setFlags() defer logging.swap(logging.newBuffers()) @@ -965,7 +965,7 @@ func TestInfoObjectRef(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { Info(tt.ref) - if !contains(severity.InfoLog, tt.want, t) { + if !contains(severity.InfoLog, tt.want) { t.Errorf("expected %v, got %v", tt.want, contents(severity.InfoLog)) } }) @@ -1465,7 +1465,7 @@ func TestLogFilter(t *testing.T) { for _, tc := range testcases { logging.newBuffers() f.logFunc(tc.args...) - got := contains(f.severity, "[FILTERED]", t) + got := contains(f.severity, "[FILTERED]") if got != tc.expectFiltered { t.Errorf("%s filter application failed, got %v, want %v", f.name, got, tc.expectFiltered) } @@ -1804,7 +1804,7 @@ func (l *testLogr) reset() { l.entries = []testLogrEntry{} } -func (l *testLogr) Info(level int, msg string, keysAndValues ...interface{}) { +func (l *testLogr) Info(_ int, msg string, keysAndValues ...interface{}) { l.mutex.Lock() defer l.mutex.Unlock() l.entries = append(l.entries, testLogrEntry{ @@ -1825,12 +1825,12 @@ func (l *testLogr) Error(err error, msg string, keysAndValues ...interface{}) { }) } -func (l *testLogr) Init(info logr.RuntimeInfo) {} -func (l *testLogr) Enabled(level int) bool { return true } +func (l *testLogr) Init(logr.RuntimeInfo) {} +func (l *testLogr) Enabled(int) bool { return true } func (l *testLogr) V(int) logr.Logger { panic("not implemented") } func (l *testLogr) WithName(string) logr.LogSink { panic("not implemented") } func (l *testLogr) WithValues(...interface{}) logr.LogSink { panic("not implemented") } -func (l *testLogr) WithCallDepth(depth int) logr.LogSink { return l } +func (l *testLogr) WithCallDepth(int) logr.LogSink { return l } var _ logr.LogSink = &testLogr{} var _ logr.CallDepthLogSink = &testLogr{} @@ -1856,7 +1856,7 @@ func (l *callDepthTestLogr) WithCallDepth(depth int) logr.LogSink { return l } -func (l *callDepthTestLogr) Info(level int, msg string, keysAndValues ...interface{}) { +func (l *callDepthTestLogr) Info(_ int, msg string, keysAndValues ...interface{}) { l.mutex.Lock() defer l.mutex.Unlock() // Add 2 to depth for the wrapper function caller and for invocation in @@ -2201,12 +2201,12 @@ func TestSettingsDeepCopy(t *testing.T) { }, }, } - copy := settings.deepCopy() - if !reflect.DeepEqual(settings, copy) { - t.Fatalf("Copy not identical to original settings. Original:\n %+v\nCopy: %+v", settings, copy) + clone := settings.deepCopy() + if !reflect.DeepEqual(settings, clone) { + t.Fatalf("Copy not identical to original settings. Original:\n %+v\nCopy: %+v", settings, clone) } settings.vmodule.filter[1].pattern = "x" - if copy.vmodule.filter[1].pattern == settings.vmodule.filter[1].pattern { + if clone.vmodule.filter[1].pattern == settings.vmodule.filter[1].pattern { t.Fatal("Copy should not have shared vmodule.filter.") } } diff --git a/klogr/klogr_test.go b/klogr/klogr_test.go index 621017d0b..0aeec030a 100644 --- a/klogr/klogr_test.go +++ b/klogr/klogr_test.go @@ -20,7 +20,7 @@ const ( ) func testOutput(t *testing.T, format string) { - new := func() logr.Logger { + createLogger := func() logr.Logger { switch format { case formatNew: return New() @@ -39,7 +39,7 @@ func testOutput(t *testing.T, format string) { expectedKlogOutput string }{ "should log with values passed to keysAndValues": { - klogr: new().V(0), + klogr: createLogger().V(0), text: "test", keysAndValues: []interface{}{"akey", "avalue"}, expectedOutput: ` "msg"="test" "akey"="avalue" @@ -48,7 +48,7 @@ func testOutput(t *testing.T, format string) { `, }, "should log with name and values passed to keysAndValues": { - klogr: new().V(0).WithName("me"), + klogr: createLogger().V(0).WithName("me"), text: "test", keysAndValues: []interface{}{"akey", "avalue"}, expectedOutput: `me "msg"="test" "akey"="avalue" @@ -57,7 +57,7 @@ func testOutput(t *testing.T, format string) { `, }, "should log with multiple names and values passed to keysAndValues": { - klogr: new().V(0).WithName("hello").WithName("world"), + klogr: createLogger().V(0).WithName("hello").WithName("world"), text: "test", keysAndValues: []interface{}{"akey", "avalue"}, expectedOutput: `hello/world "msg"="test" "akey"="avalue" @@ -66,7 +66,7 @@ func testOutput(t *testing.T, format string) { `, }, "may print duplicate keys with the same value": { - klogr: new().V(0), + klogr: createLogger().V(0), text: "test", keysAndValues: []interface{}{"akey", "avalue", "akey", "avalue"}, expectedOutput: ` "msg"="test" "akey"="avalue" @@ -75,7 +75,7 @@ func testOutput(t *testing.T, format string) { `, }, "may print duplicate keys when the values are passed to Info": { - klogr: new().V(0), + klogr: createLogger().V(0), text: "test", keysAndValues: []interface{}{"akey", "avalue", "akey", "avalue2"}, expectedOutput: ` "msg"="test" "akey"="avalue2" @@ -84,7 +84,7 @@ func testOutput(t *testing.T, format string) { `, }, "should only print the duplicate key that is passed to Info if one was passed to the logger": { - klogr: new().WithValues("akey", "avalue"), + klogr: createLogger().WithValues("akey", "avalue"), text: "test", keysAndValues: []interface{}{"akey", "avalue"}, expectedOutput: ` "msg"="test" "akey"="avalue" @@ -93,7 +93,7 @@ func testOutput(t *testing.T, format string) { `, }, "should sort within logger and parameter key/value pairs in the default format and dump the logger pairs first": { - klogr: new().WithValues("akey9", "avalue9", "akey8", "avalue8", "akey1", "avalue1"), + klogr: createLogger().WithValues("akey9", "avalue9", "akey8", "avalue8", "akey1", "avalue1"), text: "test", keysAndValues: []interface{}{"akey5", "avalue5", "akey4", "avalue4"}, expectedOutput: ` "msg"="test" "akey1"="avalue1" "akey4"="avalue4" "akey5"="avalue5" "akey8"="avalue8" "akey9"="avalue9" @@ -102,7 +102,7 @@ func testOutput(t *testing.T, format string) { `, }, "should only print the key passed to Info when one is already set on the logger": { - klogr: new().WithValues("akey", "avalue"), + klogr: createLogger().WithValues("akey", "avalue"), text: "test", keysAndValues: []interface{}{"akey", "avalue2"}, expectedOutput: ` "msg"="test" "akey"="avalue2" @@ -111,7 +111,7 @@ func testOutput(t *testing.T, format string) { `, }, "should correctly handle odd-numbers of KVs": { - klogr: new(), + klogr: createLogger(), text: "test", keysAndValues: []interface{}{"akey", "avalue", "akey2"}, expectedOutput: ` "msg"="test" "akey"="avalue" "akey2"="(MISSING)" @@ -120,7 +120,7 @@ func testOutput(t *testing.T, format string) { `, }, "should correctly handle odd-numbers of KVs in WithValue": { - klogr: new().WithValues("keyWithoutValue"), + klogr: createLogger().WithValues("keyWithoutValue"), text: "test", keysAndValues: []interface{}{"akey", "avalue", "akey2"}, // klogr format sorts all key/value pairs. @@ -130,7 +130,7 @@ func testOutput(t *testing.T, format string) { `, }, "should correctly html characters": { - klogr: new(), + klogr: createLogger(), text: "test", keysAndValues: []interface{}{"akey", "<&>"}, expectedOutput: ` "msg"="test" "akey"="<&>" @@ -139,7 +139,7 @@ func testOutput(t *testing.T, format string) { `, }, "should correctly handle odd-numbers of KVs in both log values and Info args": { - klogr: new().WithValues("basekey1", "basevar1", "basekey2"), + klogr: createLogger().WithValues("basekey1", "basevar1", "basekey2"), text: "test", keysAndValues: []interface{}{"akey", "avalue", "akey2"}, // klogr format sorts all key/value pairs. @@ -149,7 +149,7 @@ func testOutput(t *testing.T, format string) { `, }, "should correctly print regular error types": { - klogr: new().V(0), + klogr: createLogger().V(0), text: "test", keysAndValues: []interface{}{"err", errors.New("whoops")}, expectedOutput: ` "msg"="test" "err"="whoops" @@ -158,7 +158,7 @@ func testOutput(t *testing.T, format string) { `, }, "should use MarshalJSON in the default format if an error type implements it": { - klogr: new().V(0), + klogr: createLogger().V(0), text: "test", keysAndValues: []interface{}{"err", &customErrorJSON{"whoops"}}, expectedOutput: ` "msg"="test" "err"="WHOOPS" @@ -167,7 +167,7 @@ func testOutput(t *testing.T, format string) { `, }, "should correctly print regular error types when using logr.Error": { - klogr: new().V(0), + klogr: createLogger().V(0), text: "test", err: errors.New("whoops"), expectedOutput: ` "msg"="test" "error"="whoops" diff --git a/ktesting/testinglogger.go b/ktesting/testinglogger.go index 14a22bf40..ae30f0581 100644 --- a/ktesting/testinglogger.go +++ b/ktesting/testinglogger.go @@ -77,8 +77,8 @@ type TL interface { // later release. type NopTL struct{} -func (n NopTL) Helper() {} -func (n NopTL) Log(args ...interface{}) {} +func (n NopTL) Helper() {} +func (n NopTL) Log(...interface{}) {} var _ TL = NopTL{} diff --git a/textlogger/textlogger.go b/textlogger/textlogger.go index 857c76922..c0f916f89 100644 --- a/textlogger/textlogger.go +++ b/textlogger/textlogger.go @@ -88,7 +88,7 @@ func (l *tlogger) Enabled(level int) bool { return l.config.vstate.Enabled(verbosity.Level(level), 2+l.callDepth) } -func (l *tlogger) Info(level int, msg string, kvList ...interface{}) { +func (l *tlogger) Info(_ int, msg string, kvList ...interface{}) { l.print(nil, severity.InfoLog, msg, kvList) } @@ -145,18 +145,18 @@ func (l *tlogger) WriteKlogBuffer(data []byte) { // uses '/' characters to separate name elements. Callers should not pass '/' // in the provided name string, but this library does not actually enforce that. func (l *tlogger) WithName(name string) logr.LogSink { - new := *l + clone := *l if len(l.prefix) > 0 { - new.prefix = l.prefix + "/" + clone.prefix = l.prefix + "/" } - new.prefix += name - return &new + clone.prefix += name + return &clone } func (l *tlogger) WithValues(kvList ...interface{}) logr.LogSink { - new := *l - new.values = serialize.WithValues(l.values, kvList) - return &new + clone := *l + clone.values = serialize.WithValues(l.values, kvList) + return &clone } // KlogBufferWriter is implemented by the textlogger LogSink. From 1a0dfc5c09bcc50ec451a9254c81b7cabfe8ca8d Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 1 Jun 2023 18:06:50 +0200 Subject: [PATCH 3/3] github: run golangci-lint via action The main advantage is that issues get posted as annotations, which makes them easier to find when looking at a diff for a PR. While at it, golangci-lint gets invoked so that it runs in its default configuration plus the linters that were enabled explicitly before (misspell, gofmt, revive as replacement for golint). It also gets applied to the examples package. --- .github/workflows/lint.yml | 24 ++++++++++++++++++++++++ .github/workflows/test.yml | 12 ------------ .golangci.yaml | 6 ++++++ 3 files changed, 30 insertions(+), 12 deletions(-) create mode 100644 .github/workflows/lint.yml create mode 100644 .golangci.yaml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 000000000..b2582a791 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,24 @@ +name: Run lint + +on: [ push, pull_request ] + +permissions: + contents: read + +jobs: + lint: + strategy: + matrix: + path: + - . + - examples + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v2 + - name: Lint + uses: golangci/golangci-lint-action@v2 + with: + # version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version + version: latest + working-directory: ${{ matrix.path }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 870d1a2f7..fe8a775ae 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -20,18 +20,6 @@ jobs: go test -v -race ./... - name: Test examples run: cd examples && go test -v -race ./... - lint: - runs-on: ubuntu-latest - steps: - - name: Install Go - uses: actions/setup-go@v1 - - name: Checkout code - uses: actions/checkout@v2 - - name: Lint - run: | - docker run --rm -v `pwd`:/go/src/k8s.io/klog -w /go/src/k8s.io/klog \ - golangci/golangci-lint:v1.50.1 golangci-lint run --disable-all -v \ - -E govet -E misspell -E gofmt -E ineffassign -E golint apidiff: runs-on: ubuntu-latest if: github.base_ref diff --git a/.golangci.yaml b/.golangci.yaml new file mode 100644 index 000000000..0d77d65f0 --- /dev/null +++ b/.golangci.yaml @@ -0,0 +1,6 @@ +linters: + disable-all: true + enable: # sorted alphabetical + - gofmt + - misspell + - revive