From 29e939db7fb93b2b43c332b709c581ef3cfd211b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 2 Nov 2023 09:54:16 -0700 Subject: [PATCH] Upgrade the linter to golangci-lint@v1.55.1 The unused-parameter linter became stricter, so we adjust it to allow unused params that start with underscore. It can be nice to keep unused param names when implementing an interface sometimes, to help readers understand why it is unused in that particular implementation. --- .golangci.yaml | 94 ++++++++++--------- hack/install-linter.sh | 2 +- internal/dynamiccert/provider.go | 4 +- .../clientregistry/clientregistry.go | 4 +- .../storage/dynamic_global_secret_config.go | 4 +- .../refreshtoken/refreshtoken.go | 2 +- .../identity_transformations_test.go | 14 +-- internal/kubeclient/copied.go | 4 +- .../testutil/oidctestutil/oidctestutil.go | 4 +- internal/testutil/testlogger/stdr_copied.go | 22 ++--- internal/testutil/transcript_logger.go | 8 +- test/testlib/env.go | 2 +- 12 files changed, 86 insertions(+), 78 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 44a719632..1caff55d6 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -7,52 +7,52 @@ run: linters: disable-all: true enable: - # default linters - - errcheck - - gosimple - - govet - - ineffassign - - staticcheck - - typecheck - - unused + # default linters + - errcheck + - gosimple + - govet + - ineffassign + - staticcheck + - typecheck + - unused - # additional linters for this project (we should disable these if they get annoying). - - asciicheck - - bodyclose - - depguard - - dogsled - - exhaustive - - exportloopref - - funlen - - gochecknoglobals - - gochecknoinits - - gocritic - - gocyclo - - godot - - goheader - - goimports - - revive - - goprintffuncname - - gosec - - misspell - - nakedret - - nestif - - noctx - - nolintlint - - prealloc - - rowserrcheck - - exportloopref - - sqlclosecheck - - unconvert - - whitespace + # additional linters for this project (we should disable these if they get annoying). + - asciicheck + - bodyclose + # - depguard + - dogsled + - exhaustive + - exportloopref + - funlen + - gochecknoglobals + - gochecknoinits + - gocritic + - gocyclo + - godot + - goheader + - goimports + - revive + - goprintffuncname + - gosec + - misspell + - nakedret + - nestif + - noctx + - nolintlint + - prealloc + - rowserrcheck + - exportloopref + - sqlclosecheck + - unconvert + - whitespace issues: exclude-rules: # exclude tests from some rules for things that are useful in a testing context. - - path: _test\.go - linters: - - funlen - - gochecknoglobals + - path: _test\.go + linters: + - funlen + - gochecknoglobals linters-settings: funlen: @@ -64,7 +64,15 @@ linters-settings: # YYYY or YYYY-YYYY YEARS: \d\d\d\d(-\d\d\d\d)? template: |- - Copyright {{YEARS}} the Pinniped contributors. All Rights Reserved. - SPDX-License-Identifier: Apache-2.0 + Copyright {{YEARS}} the Pinniped contributors. All Rights Reserved. + SPDX-License-Identifier: Apache-2.0 goimports: local-prefixes: go.pinniped.dev + revive: + max-open-files: 2048 + rules: + - name: unused-parameter + arguments: + # Allow unused params that start with underscore. It can be nice to keep unused param names when implementing + # an interface sometimes, to help readers understand why it is unused in that particular implementation. + - allowRegex: "^_" diff --git a/hack/install-linter.sh b/hack/install-linter.sh index a06755c8b..042b23811 100755 --- a/hack/install-linter.sh +++ b/hack/install-linter.sh @@ -15,7 +15,7 @@ go version # so you can get the same results when running the linter locally. # Whenever the linter is updated in the CI pipelines, it should also be # updated here to make local development more convenient. -go install -v github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.2 +go install -v github.com/golangci/golangci-lint/cmd/golangci-lint@v1.55.1 golangci-lint --version echo "Finished. You may need to run 'rehash' in your current shell before using the new version (e.g. if you are using gvm)." diff --git a/internal/dynamiccert/provider.go b/internal/dynamiccert/provider.go index 560dff551..337148cd4 100644 --- a/internal/dynamiccert/provider.go +++ b/internal/dynamiccert/provider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package dynamiccert @@ -153,6 +153,6 @@ func (p *provider) RunOnce(_ context.Context) error { return nil // no-op, but we want to make sure to stay in sync with dynamiccertificates.ControllerRunner } -func (p *provider) Run(_ context.Context, workers int) { +func (p *provider) Run(_ context.Context, _workers int) { // no-op, but we want to make sure to stay in sync with dynamiccertificates.ControllerRunner } diff --git a/internal/federationdomain/clientregistry/clientregistry.go b/internal/federationdomain/clientregistry/clientregistry.go index 7dfa7d9f2..e7c7fe2d9 100644 --- a/internal/federationdomain/clientregistry/clientregistry.go +++ b/internal/federationdomain/clientregistry/clientregistry.go @@ -118,7 +118,7 @@ func (m *ClientManager) GetClient(ctx context.Context, id string) (fosite.Client // known or the DB check failed and nil if the JTI is not known. // // This functionality is not supported by the ClientManager. -func (*ClientManager) ClientAssertionJWTValid(ctx context.Context, jti string) error { +func (*ClientManager) ClientAssertionJWTValid(_ctx context.Context, _jti string) error { return fmt.Errorf("not implemented") } @@ -128,7 +128,7 @@ func (*ClientManager) ClientAssertionJWTValid(ctx context.Context, jti string) e // not be replayed due to the expiry. // // This functionality is not supported by the ClientManager. -func (*ClientManager) SetClientAssertionJWT(ctx context.Context, jti string, exp time.Time) error { +func (*ClientManager) SetClientAssertionJWT(_ctx context.Context, _jti string, _exp time.Time) error { return fmt.Errorf("not implemented") } diff --git a/internal/federationdomain/storage/dynamic_global_secret_config.go b/internal/federationdomain/storage/dynamic_global_secret_config.go index 50f997c4c..76d599a8a 100644 --- a/internal/federationdomain/storage/dynamic_global_secret_config.go +++ b/internal/federationdomain/storage/dynamic_global_secret_config.go @@ -51,13 +51,13 @@ func (d *DynamicGlobalSecretConfig) GetHMACHasher(ctx context.Context) func() ha return d.fositeConfig.GetHMACHasher(ctx) } -func (d *DynamicGlobalSecretConfig) GetGlobalSecret(ctx context.Context) ([]byte, error) { +func (d *DynamicGlobalSecretConfig) GetGlobalSecret(_ctx context.Context) ([]byte, error) { // Always call keyFunc() without ever caching its value, because that is the whole point // of this type. We want the global secret to be dynamic. return d.keyFunc(), nil } -func (d *DynamicGlobalSecretConfig) GetRotatedGlobalSecrets(ctx context.Context) ([][]byte, error) { +func (d *DynamicGlobalSecretConfig) GetRotatedGlobalSecrets(_ctx context.Context) ([][]byte, error) { // We don't support having multiple global secrets yet, but when we do we will need to implement this. return nil, nil } diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index d3abdc4f0..1897eff6a 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -77,7 +77,7 @@ func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) } -func (a *refreshTokenStorage) RevokeRefreshTokenMaybeGracePeriod(ctx context.Context, requestID string, signature string) error { +func (a *refreshTokenStorage) RevokeRefreshTokenMaybeGracePeriod(ctx context.Context, requestID string, _signature string) error { // We don't support a grace period, so always call the regular RevokeRefreshToken(). return a.RevokeRefreshToken(ctx, requestID) } diff --git a/internal/idtransform/identity_transformations_test.go b/internal/idtransform/identity_transformations_test.go index 80d8f2df2..59ed78c82 100644 --- a/internal/idtransform/identity_transformations_test.go +++ b/internal/idtransform/identity_transformations_test.go @@ -13,7 +13,7 @@ import ( type fakeNoopTransformer struct{} -func (a fakeNoopTransformer) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeNoopTransformer) Evaluate(_ctx context.Context, username string, groups []string) (*TransformationResult, error) { return &TransformationResult{ Username: username, Groups: groups, @@ -28,7 +28,7 @@ func (a fakeNoopTransformer) Source() interface{} { type fakeNilGroupTransformer struct{} -func (a fakeNilGroupTransformer) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeNilGroupTransformer) Evaluate(_ctx context.Context, username string, _groups []string) (*TransformationResult, error) { return &TransformationResult{ Username: username, Groups: nil, @@ -43,7 +43,7 @@ func (a fakeNilGroupTransformer) Source() interface{} { type fakeAppendStringTransformer struct{} -func (a fakeAppendStringTransformer) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeAppendStringTransformer) Evaluate(_ctx context.Context, username string, groups []string) (*TransformationResult, error) { newGroups := []string{} for _, group := range groups { newGroups = append(newGroups, group+":transformed") @@ -62,7 +62,7 @@ func (a fakeAppendStringTransformer) Source() interface{} { type fakeDeleteUsernameAndGroupsTransformer struct{} -func (a fakeDeleteUsernameAndGroupsTransformer) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeDeleteUsernameAndGroupsTransformer) Evaluate(_ctx context.Context, _username string, _groups []string) (*TransformationResult, error) { return &TransformationResult{ Username: "", Groups: []string{}, @@ -77,7 +77,7 @@ func (a fakeDeleteUsernameAndGroupsTransformer) Source() interface{} { type fakeAuthenticationDisallowedTransformer struct{} -func (a fakeAuthenticationDisallowedTransformer) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeAuthenticationDisallowedTransformer) Evaluate(_ctx context.Context, username string, groups []string) (*TransformationResult, error) { newGroups := []string{} for _, group := range groups { newGroups = append(newGroups, group+":disallowed") @@ -96,7 +96,7 @@ func (a fakeAuthenticationDisallowedTransformer) Source() interface{} { type fakeErrorTransformer struct{} -func (a fakeErrorTransformer) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeErrorTransformer) Evaluate(_ctx context.Context, _username string, _groups []string) (*TransformationResult, error) { return &TransformationResult{}, errors.New("unexpected catastrophic error") } @@ -108,7 +108,7 @@ type fakeTransformerWithSource struct { source string } -func (a fakeTransformerWithSource) Evaluate(ctx context.Context, username string, groups []string) (*TransformationResult, error) { +func (a fakeTransformerWithSource) Evaluate(_ctx context.Context, _username string, _groups []string) (*TransformationResult, error) { return nil, nil // not needed for this test } diff --git a/internal/kubeclient/copied.go b/internal/kubeclient/copied.go index a63144353..afaea8e6c 100644 --- a/internal/kubeclient/copied.go +++ b/internal/kubeclient/copied.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package kubeclient @@ -15,7 +15,7 @@ import ( ) // defaultServerUrlFor was copied from k8s.io/client-go/rest/url_utils.go. -func defaultServerUrlFor(config *restclient.Config) (*url.URL, string, error) { //nolint:revive +func defaultServerUrlFor(config *restclient.Config) (*url.URL, string, error) { hasCA := len(config.CAFile) != 0 || len(config.CAData) != 0 hasCert := len(config.CertFile) != 0 || len(config.CertData) != 0 defaultTLS := hasCA || hasCert || config.Insecure diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index f1c78e301..33eae1691 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -212,7 +212,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string { return u.Name } -func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string, grantedScopes []string) (*authenticators.Response, bool, error) { +func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string, _grantedScopes []string) (*authenticators.Response, bool, error) { return u.AuthenticateFunc(ctx, username, password) } @@ -220,7 +220,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } -func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, storedRefreshAttributes upstreamprovider.RefreshAttributes, idpDisplayName string) ([]string, error) { +func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, storedRefreshAttributes upstreamprovider.RefreshAttributes, _idpDisplayName string) ([]string, error) { if u.performRefreshArgs == nil { u.performRefreshArgs = make([]*PerformRefreshArgs, 0) } diff --git a/internal/testutil/testlogger/stdr_copied.go b/internal/testutil/testlogger/stdr_copied.go index af04ced64..e227f8917 100644 --- a/internal/testutil/testlogger/stdr_copied.go +++ b/internal/testutil/testlogger/stdr_copied.go @@ -105,7 +105,7 @@ func (l logger) Info(level int, msg string, kvList ...interface{}) { } } -func (l logger) Enabled(level int) bool { +func (l logger) Enabled(_level int) bool { return true } @@ -134,7 +134,7 @@ func (l logger) output(calldepth int, s string) { } } -func (l logger) V(level int) logr.LogSink { +func (l logger) V(_level int) logr.LogSink { return l.clone() } @@ -142,27 +142,27 @@ func (l logger) V(level int) logr.LogSink { // 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 logger) WithName(name string) logr.LogSink { - new := l.clone() + lgr := l.clone() if len(l.prefix) > 0 { - new.prefix = l.prefix + "/" + lgr.prefix = l.prefix + "/" } - new.prefix += name - return new + lgr.prefix += name + return lgr } // WithValues returns a new logr.Logger with the specified key-and-values // saved. func (l logger) WithValues(kvList ...interface{}) logr.LogSink { - new := l.clone() - new.values = append(new.values, kvList...) - return new + lgr := l.clone() + lgr.values = append(lgr.values, kvList...) + return lgr } -func (l logger) WithCallDepth(depth int) logr.LogSink { +func (l logger) WithCallDepth(_depth int) logr.LogSink { return l.clone() } var _ logr.LogSink = logger{} var _ logr.CallDepthLogSink = logger{} -func (l logger) Init(info logr.RuntimeInfo) {} +func (l logger) Init(_info logr.RuntimeInfo) {} diff --git a/internal/testutil/transcript_logger.go b/internal/testutil/transcript_logger.go index f11da06a2..d485bdfb5 100644 --- a/internal/testutil/transcript_logger.go +++ b/internal/testutil/transcript_logger.go @@ -1,4 +1,4 @@ -// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testutil @@ -37,7 +37,7 @@ func (log *TranscriptLogger) Transcript() []TranscriptLogMessage { return result } -func (log *TranscriptLogger) Info(level int, msg string, keysAndValues ...interface{}) { +func (log *TranscriptLogger) Info(_level int, msg string, keysAndValues ...interface{}) { log.lock.Lock() defer log.lock.Unlock() log.transcript = append(log.transcript, TranscriptLogMessage{ @@ -55,7 +55,7 @@ func (log *TranscriptLogger) Error(_ error, msg string, _ ...interface{}) { }) } -func (log *TranscriptLogger) Enabled(level int) bool { +func (log *TranscriptLogger) Enabled(_level int) bool { return true } @@ -71,4 +71,4 @@ func (log *TranscriptLogger) WithValues(_ ...interface{}) logr.LogSink { return log } -func (log *TranscriptLogger) Init(info logr.RuntimeInfo) {} +func (log *TranscriptLogger) Init(_info logr.RuntimeInfo) {} diff --git a/test/testlib/env.go b/test/testlib/env.go index ecbb432ec..74ea0ae90 100644 --- a/test/testlib/env.go +++ b/test/testlib/env.go @@ -101,7 +101,7 @@ type TestLDAPUpstream struct { TestUserUniqueIDAttributeValue string `json:"testUserUniqueIDAttributeValue"` TestUserDirectGroupsCNs []string `json:"testUserDirectGroupsCNs"` TestUserDirectPosixGroupsCNs []string `json:"testUserDirectPosixGroupsCNs"` - TestUserDirectGroupsDNs []string `json:"testUserDirectGroupsDNs"` //nolint:revive // this is "distinguished names", not "DNS" + TestUserDirectGroupsDNs []string `json:"testUserDirectGroupsDNs"` TestUserSAMAccountNameValue string `json:"testUserSAMAccountNameValue"` TestUserPrincipalNameValue string `json:"testUserPrincipalNameValue"` TestUserIndirectGroupsSAMAccountNames []string `json:"TestUserIndirectGroupsSAMAccountNames"`