From c8702914c74e9d15e35a976287f30a6d0d96e60c Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Thu, 10 Nov 2022 12:37:39 +0000 Subject: [PATCH] build: Add revive linter (#7) --- .golangci.yml | 32 ++++++++++++++++++- pkg/credentialprovider/plugin_test.go | 23 ++++++------- pkg/credentialprovider/static_credentials.go | 6 ++-- .../static_credentials_test.go | 1 + pkg/install/install.go | 18 ++++++++--- pkg/install/install_test.go | 21 ++++++------ 6 files changed, 73 insertions(+), 28 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 749d813..d7c3210 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -18,7 +18,6 @@ linters: - gci - goconst - gocritic - - gocyclo - godot - gofumpt - gosec @@ -29,6 +28,7 @@ linters: - misspell - nolintlint - prealloc + - revive - staticcheck - stylecheck - tenv @@ -71,6 +71,36 @@ linters-settings: extra-rules: true lll: line-length: 120 + revive: + enable-all-rules: true + ignore-generated-header: true + max-open-files: 2048 + severity: error + rules: + - name: add-constant + arguments: + - maxLitCount: "3" + allowStrs: '"","testdata"' + allowInts: "0,1,2" + allowFloats: "0.0,0.,1.0,1.,2.0,2." + - name: argument-limit + arguments: [4] + - name: cognitive-complexity + disabled: true + - name: function-length + arguments: [30, 0] + - name: max-public-structs + arguments: [10] + - name: file-header + disabled: true + - name: line-length-limit + disabled: true + - name: banned-characters + disabled: true + - name: cyclomatic + arguments: [10] + - name: function-result-limit + arguments: [3] issues: exclude-rules: diff --git a/pkg/credentialprovider/plugin_test.go b/pkg/credentialprovider/plugin_test.go index d406b57..58c50eb 100644 --- a/pkg/credentialprovider/plugin_test.go +++ b/pkg/credentialprovider/plugin_test.go @@ -17,13 +17,14 @@ import ( type fakePlugin struct{} -func (f *fakePlugin) GetCredentials( - ctx context.Context, - image string, - args []string, +func (fakePlugin) GetCredentials( + _ context.Context, + _ string, + _ []string, ) (*v1alpha1.CredentialProviderResponse, error) { return &v1alpha1.CredentialProviderResponse{ - CacheKeyType: v1alpha1.RegistryPluginCacheKeyType, + CacheKeyType: v1alpha1.RegistryPluginCacheKeyType, + //nolint:revive // Dummy value in test file, no need for const. CacheDuration: &metav1.Duration{Duration: 10 * time.Minute}, Auth: map[string]v1alpha1.AuthConfig{ "*.registry.io": { @@ -45,27 +46,27 @@ func Test_runPlugin(t *testing.T) { }{ { name: "successful test case", - //nolint:lll // just a long string + //nolint:lll // Just a long string. req: `{"kind":"CredentialProviderRequest","apiVersion":"credentialprovider.kubelet.k8s.io/v1alpha1","image":"test.registry.io/foobar"}`, - //nolint:lll // just a long string + //nolint:lll // Just a long string. expectedOut: `{"kind":"CredentialProviderResponse","apiVersion":"credentialprovider.kubelet.k8s.io/v1alpha1","cacheKeyType":"Registry","cacheDuration":"10m0s","auth":{"*.registry.io":{"username":"user","password":"password"}}} `, }, { name: "invalid kind", - //nolint:lll // just a long string + //nolint:lll // Just a long string. req: `{"kind":"CredentialProviderFoo","apiVersion":"credentialprovider.kubelet.k8s.io/v1alpha1","image":"test.registry.io/foobar"}`, expectErr: ErrUnsupportedRequestKind, }, { name: "invalid apiVersion", - //nolint:lll // just a long string + //nolint:lll // Just a long string. req: `{"kind":"CredentialProviderRequest","apiVersion":"foo.k8s.io/v1alpha1","image":"test.registry.io/foobar"}`, expectErr: ErrUnsupportedAPIVersion, }, { name: "empty image", - //nolint:lll // just a long string + //nolint:lll // Just a long string. req: `{"kind":"CredentialProviderRequest","apiVersion":"credentialprovider.kubelet.k8s.io/v1alpha1","image":""}`, expectErr: ErrEmptyImageInRequest, }, @@ -76,7 +77,7 @@ func Test_runPlugin(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p := NewCredentialProvider(&fakePlugin{}) + p := NewCredentialProvider(fakePlugin{}) out := &bytes.Buffer{} require.ErrorIs( diff --git a/pkg/credentialprovider/static_credentials.go b/pkg/credentialprovider/static_credentials.go index 2f64e96..8e6bc1e 100644 --- a/pkg/credentialprovider/static_credentials.go +++ b/pkg/credentialprovider/static_credentials.go @@ -23,9 +23,9 @@ func NewStaticProvider(credentialsFile string) (CredentialProvider, error) { // GetCredentials will ignore the image and args arguments and simply read a credentials file and return its content. func (s staticProvider) GetCredentials( - ctx context.Context, - image string, - args []string, + _ context.Context, + _ string, + _ []string, ) (response *v1alpha1.CredentialProviderResponse, err error) { credentials, err := os.ReadFile(s.credentialsFile) if err != nil { diff --git a/pkg/credentialprovider/static_credentials_test.go b/pkg/credentialprovider/static_credentials_test.go index bf333b2..228ef53 100644 --- a/pkg/credentialprovider/static_credentials_test.go +++ b/pkg/credentialprovider/static_credentials_test.go @@ -70,6 +70,7 @@ func TestGetCredentials(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() credentialsFile := path.Join(t.TempDir(), "image-credentials.json") + //nolint:revive // Dummy value in test file, no need for const. err := os.WriteFile(credentialsFile, []byte(tt.credentialsString), 0o600) require.NoError(t, err, "error writing temporary credentials file") diff --git a/pkg/install/install.go b/pkg/install/install.go index db22875..4a5cacc 100644 --- a/pkg/install/install.go +++ b/pkg/install/install.go @@ -15,6 +15,14 @@ import ( "go.etcd.io/etcd/client/pkg/v3/fileutil" ) +//nolint:gosec // None of these are security credentials. +const ( + CredentialProviderSourceDirEnvVar = "CREDENTIAL_PROVIDER_SOURCE_DIR" + CredentialProviderTargetDirEnvVar = "CREDENTIAL_PROVIDER_TARGET_DIR" + SkipCredentialProviderBinariesEnvVar = "SKIP_CREDENTIAL_PROVIDER_BINARIES" + UpdateCredentialProviderBinariesEnvVar = "UPDATE_CREDENTIAL_PROVIDER_BINARIES" +) + type config struct { // SkipCredentialProviderBinaries is a comma-separated list of binaries. Each binary in the list. // will be skipped when installing to the host. @@ -25,11 +33,11 @@ type config struct { UpdateCredentialProviderBinaries bool `envconfig:"UPDATE_CREDENTIAL_PROVIDER_BINARIES" default:"true"` // CredentialProviderSourceDir controls where to read the binaries from to copy to the host. - //nolint:lll // This is just a long default dir. + //nolint:lll // Long struct tag value. CredentialProviderSourceDir string `envconfig:"CREDENTIAL_PROVIDER_SOURCE_DIR" default:"/opt/image-credential-provider/bin/"` // CredentialProviderTargetDir controls where to place the binaries on the host. - //nolint:lll // This is just a long default dir. + //nolint:lll // Long struct tag value. CredentialProviderTargetDir string `envconfig:"CREDENTIAL_PROVIDER_TARGET_DIR" default:"/host/etc/kubernetes/image-credential-provider/"` } @@ -105,7 +113,7 @@ func Install(logger logrus.FieldLogger) error { } // copyFileAndPermissions copies file permission. -func copyFileAndPermissions(src, dst string, logger logrus.FieldLogger) (err error) { +func copyFileAndPermissions(src, dst string, logger logrus.FieldLogger) error { // If the source and destination are the same, we can simply return. skip, err := destinationUpToDate(src, dst, logger) if err != nil { @@ -129,12 +137,14 @@ func copyFileAndPermissions(src, dst string, logger logrus.FieldLogger) (err err return fmt.Errorf("failed to rename file: %s", err) } - return + return nil } // destinationUptoDate compares the given files and returns // whether or not the destination file needs to be updated with the // contents of the source file. +// +//nolint:revive // Easy to read func that just does a lot, ignore cyclomatic complexity. func destinationUpToDate(src, dst string, logger logrus.FieldLogger) (bool, error) { // Stat the src file. f1info, err := os.Stat(src) diff --git a/pkg/install/install_test.go b/pkg/install/install_test.go index b5c1d24..3df7e1b 100644 --- a/pkg/install/install_test.go +++ b/pkg/install/install_test.go @@ -60,8 +60,8 @@ func assertFileHashesDifferent(t *testing.T, a, b string) { func TestSuccessfulCopy(t *testing.T) { tmpDir := t.TempDir() - t.Setenv("CREDENTIAL_PROVIDER_SOURCE_DIR", "testdata") - t.Setenv("CREDENTIAL_PROVIDER_TARGET_DIR", tmpDir) + t.Setenv(install.CredentialProviderSourceDirEnvVar, "testdata") + t.Setenv(install.CredentialProviderTargetDirEnvVar, tmpDir) require.NoError(t, install.Install(logrus.New())) @@ -84,9 +84,11 @@ func TestSuccessfulCopy(t *testing.T) { func TestSuccessfulCopySkipFile(t *testing.T) { tmpDir := t.TempDir() - t.Setenv("CREDENTIAL_PROVIDER_SOURCE_DIR", "testdata") - t.Setenv("CREDENTIAL_PROVIDER_TARGET_DIR", tmpDir) - t.Setenv("SKIP_CREDENTIAL_PROVIDER_BINARIES", "dummybinary2") + const dummyBinary2Name = "dummybinary2" + + t.Setenv(install.CredentialProviderSourceDirEnvVar, "testdata") + t.Setenv(install.CredentialProviderTargetDirEnvVar, tmpDir) + t.Setenv(install.SkipCredentialProviderBinariesEnvVar, dummyBinary2Name) require.NoError(t, install.Install(logrus.New())) @@ -96,7 +98,7 @@ func TestSuccessfulCopySkipFile(t *testing.T) { for _, f := range testFiles { destFile := filepath.Join(tmpDir, f.Name()) - if f.Name() == "dummybinary2" { + if f.Name() == dummyBinary2Name { assert.NoFileExists(t, destFile) continue } @@ -115,12 +117,13 @@ func TestSuccessfulCopySkipFile(t *testing.T) { func TestSuccessfulCopyDoNotUpdate(t *testing.T) { tmpDir := t.TempDir() - t.Setenv("CREDENTIAL_PROVIDER_SOURCE_DIR", "testdata") - t.Setenv("CREDENTIAL_PROVIDER_TARGET_DIR", tmpDir) - t.Setenv("UPDATE_CREDENTIAL_PROVIDER_BINARIES", "false") + t.Setenv(install.CredentialProviderSourceDirEnvVar, "testdata") + t.Setenv(install.CredentialProviderTargetDirEnvVar, tmpDir) + t.Setenv(install.UpdateCredentialProviderBinariesEnvVar, "false") assert.NoError( t, + //nolint:revive // Dummy value in test file, no need for const. os.WriteFile(filepath.Join(tmpDir, "dummybinary2"), []byte("differentcontent"), 0o600), )