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 48f0aca97..b798c1b75 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 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 323dd64b3..7aff88925 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/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/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..61d26a5c0 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{}) ) @@ -259,36 +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") } -// 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 { +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/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 f42d8c9a0..dc270cc84 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 { @@ -912,7 +910,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 { @@ -922,20 +920,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) } } } @@ -961,7 +959,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() @@ -1117,7 +1115,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{} @@ -1125,7 +1123,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{} } @@ -1216,8 +1214,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 { @@ -1296,9 +1294,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 21b4dd127..3f427dde1 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 } @@ -93,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) } @@ -109,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") } } @@ -182,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") } } @@ -240,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") } } @@ -264,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") } } @@ -287,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") } } @@ -308,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") } } @@ -325,12 +324,12 @@ 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) { + 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") } } @@ -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") } @@ -351,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") } } @@ -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) @@ -376,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()) @@ -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") } @@ -966,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)) } }) @@ -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 { @@ -1466,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) } @@ -1805,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{ @@ -1826,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{} @@ -1857,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 @@ -2202,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.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..0aeec030a 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" @@ -19,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() @@ -38,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" @@ -47,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" @@ -56,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" @@ -65,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" @@ -74,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" @@ -83,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" @@ -92,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" @@ -101,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" @@ -110,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)" @@ -119,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. @@ -129,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"="<&>" @@ -138,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. @@ -148,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" @@ -157,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" @@ -166,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" @@ -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 868446c39..2119a2226 100644 --- a/ktesting/options.go +++ b/ktesting/options.go @@ -120,7 +120,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.go b/ktesting/testinglogger.go index da0e2b81a..37d5ca694 100644 --- a/ktesting/testinglogger.go +++ b/ktesting/testinglogger.go @@ -62,8 +62,8 @@ type TL interface { // output in memory is relevant. 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/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 14cfa8088..7ae8d9378 100644 --- a/textlogger/options.go +++ b/textlogger/options.go @@ -107,7 +107,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 90b265003..08235ab9c 100644 --- a/textlogger/textlogger.go +++ b/textlogger/textlogger.go @@ -73,7 +73,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) } @@ -119,29 +119,29 @@ 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 // 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.