From cb0ada1e1cbd94c8141388fa821850ce5f70bc54 Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Tue, 25 Apr 2023 11:27:27 +0800 Subject: [PATCH] Enable staticcheck and resolve found issues. Signed-off-by: Xun Jiang --- .github/workflows/pr-ci-check.yml | 4 ---- changelogs/unreleased/6183-blackpiglet | 1 + go.mod | 2 +- golangci.yaml | 15 +++++++++++++++ pkg/backup/backup.go | 6 ++---- pkg/backup/backup_test.go | 1 - pkg/backup/item_collector.go | 4 ++++ pkg/cmd/cli/backup/create_test.go | 4 ++-- pkg/cmd/cli/select_option.go | 7 ++++--- pkg/cmd/cli/version/version_test.go | 2 +- pkg/cmd/server/server.go | 4 ++++ pkg/cmd/util/output/output.go | 4 ---- pkg/controller/backup_sync_controller_test.go | 13 ------------- .../pod_volume_backup_controller_test.go | 4 +++- pkg/controller/restore_operations_controller.go | 1 + pkg/persistence/object_store_test.go | 4 ++-- pkg/plugin/framework/common/server_errors.go | 1 + pkg/podvolume/backupper.go | 2 +- pkg/staticcheck.conf | 1 - pkg/test/discovery_client.go | 5 ++++- pkg/test/fake_controller_runtime_client.go | 2 +- pkg/uploader/provider/kopia_test.go | 4 ++-- pkg/uploader/provider/restic_test.go | 4 ++-- test/staticcheck.conf | 1 - 24 files changed, 51 insertions(+), 45 deletions(-) create mode 100644 changelogs/unreleased/6183-blackpiglet delete mode 100644 pkg/staticcheck.conf delete mode 100644 test/staticcheck.conf diff --git a/.github/workflows/pr-ci-check.yml b/.github/workflows/pr-ci-check.yml index 288418cb8..3df5c4684 100644 --- a/.github/workflows/pr-ci-check.yml +++ b/.github/workflows/pr-ci-check.yml @@ -29,7 +29,3 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} files: coverage.out verbose: true - - name: Run staticcheck - uses: dominikh/staticcheck-action@v1.3.0 - with: - version: "2022.1.3" \ No newline at end of file diff --git a/changelogs/unreleased/6183-blackpiglet b/changelogs/unreleased/6183-blackpiglet new file mode 100644 index 000000000..77df054e8 --- /dev/null +++ b/changelogs/unreleased/6183-blackpiglet @@ -0,0 +1 @@ +Enable staticcheck and resolve found issues \ No newline at end of file diff --git a/go.mod b/go.mod index 5f926abf2..4a20b8913 100644 --- a/go.mod +++ b/go.mod @@ -38,6 +38,7 @@ require ( golang.org/x/net v0.7.0 golang.org/x/oauth2 v0.0.0-20220608161450-d0670ef3b1eb golang.org/x/sync v0.0.0-20210220032951-036812b2e83c + golang.org/x/text v0.7.0 google.golang.org/api v0.74.0 google.golang.org/grpc v1.45.0 google.golang.org/protobuf v1.28.0 @@ -135,7 +136,6 @@ require ( golang.org/x/exp v0.0.0-20210916165020-5cb4fee858ee // indirect golang.org/x/sys v0.5.0 // indirect golang.org/x/term v0.5.0 // indirect - golang.org/x/text v0.7.0 // indirect golang.org/x/time v0.0.0-20220609170525-579cf78fd858 // indirect golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect diff --git a/golangci.yaml b/golangci.yaml index 5b4bfbd64..6ba6f0057 100644 --- a/golangci.yaml +++ b/golangci.yaml @@ -20,6 +20,7 @@ run: # on Windows. skip-dirs: - test/e2e/* + - pkg/plugin/generated/* # - autogenerated_by_my_lib # default is true. Enables skipping of directories: @@ -249,6 +250,8 @@ linters-settings: rowserrcheck: packages: - github.com/jmoiron/sqlx + staticcheck: + testpackage: # regexp pattern to skip files skip-regexp: (export|internal)_test\.go @@ -297,6 +300,7 @@ linters: - goimports - gosec - misspell + - staticcheck - typecheck - unparam - unused @@ -306,6 +310,17 @@ linters: issues: + exclude-rules: + - linters: + - staticcheck + text: "github.com/golang/protobuf/proto" # grpc-go still uses github.com/golang/protobuf/proto. + - linters: + - staticcheck + text: "github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2019-06-01/storage" # Kopia still depends on this. + - linters: + - staticcheck + text: "DefaultVolumesToRestic" # No need to report deprecate for DefaultVolumesToRestic. + # The list of ids of default excludes to include or disable. By default it's empty. include: - EXC0002 # disable excluding of issues about comments from golint diff --git a/pkg/backup/backup.go b/pkg/backup/backup.go index c7492c0c4..34cb42baa 100644 --- a/pkg/backup/backup.go +++ b/pkg/backup/backup.go @@ -346,7 +346,6 @@ func (kb *kubernetesBackupper) BackupWithResolvers(log logrus.FieldLogger, }() backedUpGroupResources := map[schema.GroupResource]bool{} - totalItems := len(items) for i, item := range items { log.WithFields(map[string]interface{}{ @@ -381,7 +380,7 @@ func (kb *kubernetesBackupper) BackupWithResolvers(log logrus.FieldLogger, // updated total is computed as "how many items we've backed up so far, plus // how many items we know of that are remaining" - totalItems = len(backupRequest.BackedUpItems) + (len(items) - (i + 1)) + totalItems := len(backupRequest.BackedUpItems) + (len(items) - (i + 1)) // send a progress update update <- progressUpdate{ @@ -587,7 +586,6 @@ func (kb *kubernetesBackupper) FinalizeBackup(log logrus.FieldLogger, } updateFiles := make(map[string]FileForArchive) backedUpGroupResources := map[schema.GroupResource]bool{} - totalItems := len(items) for i, item := range items { log.WithFields(map[string]interface{}{ @@ -626,7 +624,7 @@ func (kb *kubernetesBackupper) FinalizeBackup(log logrus.FieldLogger, // updated total is computed as "how many items we've backed up so far, plus // how many items we know of that are remaining" - totalItems = len(backupRequest.BackedUpItems) + (len(items) - (i + 1)) + totalItems := len(backupRequest.BackedUpItems) + (len(items) - (i + 1)) log.WithFields(map[string]interface{}{ "progress": "", diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index f14375549..07e8df6ea 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -107,7 +107,6 @@ func TestBackedUpItemsMatchesTarballContents(t *testing.T) { if item.namespace != "" { fileWithVersion = fileWithVersion + "/v1-preferredversion/" + "namespaces/" + item.namespace } else { - file = file + "/cluster" fileWithVersion = fileWithVersion + "/v1-preferredversion" + "/cluster" } fileWithVersion = fileWithVersion + "/" + item.name + ".json" diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index 08ee44fb1..373cbaa55 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -225,6 +225,10 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group }, ).Infof("Getting item") resourceClient, err := r.dynamicFactory.ClientForGroupVersionResource(gv, resource, resourceID.Namespace) + if err != nil { + log.WithError(errors.WithStack(err)).Error("Error getting client for resource") + continue + } unstructured, err := resourceClient.Get(resourceID.Name, metav1.GetOptions{}) if err != nil { log.WithError(errors.WithStack(err)).Error("Error getting item") diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 5e7b0161b..0966165b5 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -105,10 +105,10 @@ func TestCreateOptions_BuildBackupFromSchedule(t *testing.T) { } func TestCreateOptions_OrderedResources(t *testing.T) { - orderedResources, err := ParseOrderedResources("pods= ns1/p1; ns1/p2; persistentvolumeclaims=ns2/pvc1, ns2/pvc2") + _, err := ParseOrderedResources("pods= ns1/p1; ns1/p2; persistentvolumeclaims=ns2/pvc1, ns2/pvc2") assert.NotNil(t, err) - orderedResources, err = ParseOrderedResources("pods= ns1/p1,ns1/p2 ; persistentvolumeclaims=ns2/pvc1,ns2/pvc2") + orderedResources, err := ParseOrderedResources("pods= ns1/p1,ns1/p2 ; persistentvolumeclaims=ns2/pvc1,ns2/pvc2") assert.NoError(t, err) expectedResources := map[string]string{ diff --git a/pkg/cmd/cli/select_option.go b/pkg/cmd/cli/select_option.go index 45ed54716..b0f18f4ef 100644 --- a/pkg/cmd/cli/select_option.go +++ b/pkg/cmd/cli/select_option.go @@ -18,9 +18,10 @@ package cli import ( "errors" - "strings" "github.com/spf13/pflag" + "golang.org/x/text/cases" + "golang.org/x/text/language" "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" ) @@ -64,6 +65,6 @@ func (o *SelectOptions) Validate() error { // BindFlags binds options for this command to flags. func (o *SelectOptions) BindFlags(flags *pflag.FlagSet) { - flags.BoolVar(&o.All, "all", o.All, strings.Title(o.CMD)+" all "+o.SingularTypeName+"s") - flags.VarP(&o.Selector, "selector", "l", strings.Title(o.CMD)+" all "+o.SingularTypeName+"s matching this label selector.") + flags.BoolVar(&o.All, "all", o.All, cases.Title(language.Und).String(o.CMD)+" all "+o.SingularTypeName+"s") + flags.VarP(&o.Selector, "selector", "l", cases.Title(language.Und).String(o.CMD)+" all "+o.SingularTypeName+"s matching this label selector.") } diff --git a/pkg/cmd/cli/version/version_test.go b/pkg/cmd/cli/version/version_test.go index 40165dfbc..90e6e811b 100644 --- a/pkg/cmd/cli/version/version_test.go +++ b/pkg/cmd/cli/version/version_test.go @@ -83,7 +83,7 @@ func TestPrintVersion(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { var ( - kbClient = fake.NewFakeClientWithScheme(scheme.Scheme) + kbClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() serverStatusGetter = new(mockServerStatusGetter) buf = new(bytes.Buffer) ) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index b65cd4680..028004e6a 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -340,6 +340,10 @@ func newServer(f client.Factory, config serverConfig, logger *logrus.Logger) (*s } credentialSecretStore, err := credentials.NewNamespacedSecretStore(mgr.GetClient(), f.Namespace()) + if err != nil { + cancelFunc() + return nil, err + } s := &server{ namespace: f.Namespace(), diff --git a/pkg/cmd/util/output/output.go b/pkg/cmd/util/output/output.go index 1a520b0ae..d0908c2eb 100644 --- a/pkg/cmd/util/output/output.go +++ b/pkg/cmd/util/output/output.go @@ -219,10 +219,6 @@ func printTable(cmd *cobra.Command, obj runtime.Object) (bool, error) { return false, errors.Errorf("type %T is not supported", obj) } - if table == nil { - return false, errors.Errorf("error generating table for type %T", obj) - } - // 2. print table tablePrinter, err := NewPrinter(cmd) if err != nil { diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index 14f076cf4..bc362edb4 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -32,7 +32,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" - core "k8s.io/client-go/testing" testclocks "k8s.io/utils/clock/testing" ctrl "sigs.k8s.io/controller-runtime" @@ -669,22 +668,10 @@ var _ = Describe("Backup Sync Reconciler", func() { logger: velerotest.NewLogger(), } - expectedDeleteActions := make([]core.Action, 0) - for _, backup := range test.k8sBackups { // add test backup to client err := client.Create(context.TODO(), backup, &ctrlClient.CreateOptions{}) Expect(err).ShouldNot(HaveOccurred()) - - // if we expect this backup to be deleted, set up the expected DeleteAction - if test.expectedDeletes.Has(backup.Name) { - actionDelete := core.NewDeleteAction( - velerov1api.SchemeGroupVersion.WithResource("backups"), - test.namespace, - backup.Name, - ) - expectedDeleteActions = append(expectedDeleteActions, actionDelete) - } } bslName := "default" diff --git a/pkg/controller/pod_volume_backup_controller_test.go b/pkg/controller/pod_volume_backup_controller_test.go index b09017a71..f940f67e3 100644 --- a/pkg/controller/pod_volume_backup_controller_test.go +++ b/pkg/controller/pod_volume_backup_controller_test.go @@ -114,7 +114,7 @@ var _ = Describe("PodVolumeBackup Reconciler", func() { func(test request) { ctx := context.Background() - fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) + fakeClient := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() err = fakeClient.Create(ctx, test.pvb) Expect(err).To(BeNil()) @@ -139,6 +139,8 @@ var _ = Describe("PodVolumeBackup Reconciler", func() { fakeFS, ) + Expect(err).To(BeNil()) + // Setup reconciler Expect(velerov1api.AddToScheme(scheme.Scheme)).To(Succeed()) r := PodVolumeBackupReconciler{ diff --git a/pkg/controller/restore_operations_controller.go b/pkg/controller/restore_operations_controller.go index 1677d97c9..8f5945742 100644 --- a/pkg/controller/restore_operations_controller.go +++ b/pkg/controller/restore_operations_controller.go @@ -218,6 +218,7 @@ func (r *restoreOperationsReconciler) updateRestoreAndOperationsJSON( completionChanges bool) error { if len(operations.ErrsSinceUpdate) > 0 { // FIXME: download/upload results + r.logger.WithField("restore", restore.Name).Infof("Restore has %d errors", len(operations.ErrsSinceUpdate)) } removeIfComplete := true defer func() { diff --git a/pkg/persistence/object_store_test.go b/pkg/persistence/object_store_test.go index e972cebc8..896e5d435 100644 --- a/pkg/persistence/object_store_test.go +++ b/pkg/persistence/object_store_test.go @@ -412,7 +412,7 @@ func TestGetBackupVolumeSnapshots(t *testing.T) { // volumesnapshots file containing invalid data should error harness.objectStore.PutObject(harness.bucket, "backups/test-backup/test-backup-volumesnapshots.json.gz", newStringReadSeeker("foo")) - res, err = harness.GetBackupVolumeSnapshots("test-backup") + _, err = harness.GetBackupVolumeSnapshots("test-backup") assert.NotNil(t, err) // volumesnapshots file containing gzipped json data should return correctly @@ -454,7 +454,7 @@ func TestGetBackupItemOperations(t *testing.T) { // itemoperations file containing invalid data should error harness.objectStore.PutObject(harness.bucket, "backups/test-backup/test-backup-itemoperations.json.gz", newStringReadSeeker("foo")) - res, err = harness.GetBackupItemOperations("test-backup") + _, err = harness.GetBackupItemOperations("test-backup") assert.NotNil(t, err) // itemoperations file containing gzipped json data should return correctly diff --git a/pkg/plugin/framework/common/server_errors.go b/pkg/plugin/framework/common/server_errors.go index 7763d4e9e..1dcc945f6 100644 --- a/pkg/plugin/framework/common/server_errors.go +++ b/pkg/plugin/framework/common/server_errors.go @@ -17,6 +17,7 @@ limitations under the License. package common import ( + //lint:ignore SA1019 grpc-go still depends on github.com/golang/protobuf/proto goproto "github.com/golang/protobuf/proto" "github.com/pkg/errors" "google.golang.org/grpc/codes" diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index cc62aaaca..83974fd89 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -229,7 +229,7 @@ func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api. } volumeBackup := newPodVolumeBackup(backup, pod, volume, repo.Spec.ResticIdentifier, b.uploaderType, pvc) - if volumeBackup, err = b.veleroClient.VeleroV1().PodVolumeBackups(volumeBackup.Namespace).Create(context.TODO(), volumeBackup, metav1.CreateOptions{}); err != nil { + if _, err = b.veleroClient.VeleroV1().PodVolumeBackups(volumeBackup.Namespace).Create(context.TODO(), volumeBackup, metav1.CreateOptions{}); err != nil { errs = append(errs, err) continue } diff --git a/pkg/staticcheck.conf b/pkg/staticcheck.conf deleted file mode 100644 index ba239a708..000000000 --- a/pkg/staticcheck.conf +++ /dev/null @@ -1 +0,0 @@ -checks = [] \ No newline at end of file diff --git a/pkg/test/discovery_client.go b/pkg/test/discovery_client.go index fe06af635..e492aafc9 100644 --- a/pkg/test/discovery_client.go +++ b/pkg/test/discovery_client.go @@ -19,6 +19,9 @@ package test import ( "strings" + "golang.org/x/text/cases" + "golang.org/x/text/language" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/discovery" discoveryfake "k8s.io/client-go/discovery/fake" @@ -73,7 +76,7 @@ func (c *DiscoveryClient) WithAPIResource(resource *APIResource) *DiscoveryClien Namespaced: resource.Namespaced, Group: resource.Group, Version: resource.Version, - Kind: strings.Title(strings.TrimSuffix(resource.Name, "s")), + Kind: cases.Title(language.Und).String(strings.TrimSuffix(resource.Name, "s")), Verbs: metav1.Verbs([]string{"list", "create", "get", "delete"}), ShortNames: []string{resource.ShortName}, }) diff --git a/pkg/test/fake_controller_runtime_client.go b/pkg/test/fake_controller_runtime_client.go index 0be391bd9..97487d9ab 100644 --- a/pkg/test/fake_controller_runtime_client.go +++ b/pkg/test/fake_controller_runtime_client.go @@ -48,5 +48,5 @@ func NewFakeControllerRuntimeClient(t *testing.T, initObjs ...runtime.Object) cl require.NoError(t, err) err = snapshotv1api.AddToScheme(scheme) require.NoError(t, err) - return k8sfake.NewFakeClientWithScheme(scheme, initObjs...) + return k8sfake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(initObjs...).Build() } diff --git a/pkg/uploader/provider/kopia_test.go b/pkg/uploader/provider/kopia_test.go index 955bf83f4..c9a2b6ee1 100644 --- a/pkg/uploader/provider/kopia_test.go +++ b/pkg/uploader/provider/kopia_test.go @@ -37,7 +37,7 @@ import ( func TestRunBackup(t *testing.T) { var kp kopiaProvider kp.log = logrus.New() - updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: kp.log, Ctx: context.Background(), Cli: fake.NewFakeClientWithScheme(scheme.Scheme)} + updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: kp.log, Ctx: context.Background(), Cli: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()} testCases := []struct { name string hookBackupFunc func(ctx context.Context, fsUploader *snapshotfs.Uploader, repoWriter repo.RepositoryWriter, sourcePath, parentSnapshot string, log logrus.FieldLogger) (*uploader.SnapshotInfo, bool, error) @@ -81,7 +81,7 @@ func TestRunBackup(t *testing.T) { func TestRunRestore(t *testing.T) { var kp kopiaProvider kp.log = logrus.New() - updater := FakeRestoreProgressUpdater{PodVolumeRestore: &velerov1api.PodVolumeRestore{}, Log: kp.log, Ctx: context.Background(), Cli: fake.NewFakeClientWithScheme(scheme.Scheme)} + updater := FakeRestoreProgressUpdater{PodVolumeRestore: &velerov1api.PodVolumeRestore{}, Log: kp.log, Ctx: context.Background(), Cli: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()} testCases := []struct { name string diff --git a/pkg/uploader/provider/restic_test.go b/pkg/uploader/provider/restic_test.go index 042602777..16d838225 100644 --- a/pkg/uploader/provider/restic_test.go +++ b/pkg/uploader/provider/restic_test.go @@ -34,7 +34,7 @@ import ( func TestResticRunBackup(t *testing.T) { var rp resticProvider rp.log = logrus.New() - updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: rp.log, Ctx: context.Background(), Cli: fake.NewFakeClientWithScheme(scheme.Scheme)} + updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: rp.log, Ctx: context.Background(), Cli: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()} testCases := []struct { name string hookBackupFunc func(repoIdentifier string, passwordFile string, path string, tags map[string]string) *restic.Command @@ -74,7 +74,7 @@ func TestResticRunBackup(t *testing.T) { func TestResticRunRestore(t *testing.T) { var rp resticProvider rp.log = logrus.New() - updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: rp.log, Ctx: context.Background(), Cli: fake.NewFakeClientWithScheme(scheme.Scheme)} + updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: rp.log, Ctx: context.Background(), Cli: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()} ResticRestoreCMDFunc = func(repoIdentifier, passwordFile, snapshotID, target string) *restic.Command { return &restic.Command{Args: []string{""}} } diff --git a/test/staticcheck.conf b/test/staticcheck.conf deleted file mode 100644 index ba239a708..000000000 --- a/test/staticcheck.conf +++ /dev/null @@ -1 +0,0 @@ -checks = [] \ No newline at end of file