Merge pull request #6183 from blackpiglet/linter_staticcheck

Enable staticcheck and resolve found issues.

.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"

changelogs/unreleased/6183-blackpiglet

@@ -0,0 +1 @@
+Enable staticcheck and resolve found issues

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

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
@@ -298,6 +301,7 @@ linters:
     - gosec
     - govet
     - misspell
+    - staticcheck
     - stylecheck
     - typecheck
     - unparam
@@ -308,6 +312,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

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":  "",

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")

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{

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.")
 }

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)
 			)

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(),

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 {

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"

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{

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() {

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

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"

pkg/podvolume/backupper.go

@@ -234,7 +234,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
 		}

pkg/staticcheck.conf

@@ -1 +0,0 @@
-checks = []

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},
 	})

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()
 }

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

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{""}}
 	}

test/staticcheck.conf

@@ -1 +0,0 @@
-checks = []